[stable-3.16] Bugfix/read only windows folders

src/common/filesystembase.cpp

@@ -245,54 +245,6 @@ bool FileSystem::rename(const QString &originFileName,
     return success;
 }
 
-bool FileSystem::uncheckedRenameReplace(const QString &originFileName,
-    const QString &destinationFileName,
-    QString *errorString)
-{
-#ifndef Q_OS_WIN
-    bool success = false;
-    QFile orig(originFileName);
-    // We want a rename that also overwrites.  QFile::rename does not overwrite.
-    // Qt 5.1 has QSaveFile::renameOverwrite we could use.
-    // ### FIXME
-    success = true;
-    bool destExists = fileExists(destinationFileName);
-    if (destExists && !QFile::remove(destinationFileName)) {
-        *errorString = orig.errorString();
-        qCWarning(lcFileSystem) << "Target file could not be removed.";
-        success = false;
-    }
-    if (success) {
-        success = orig.rename(destinationFileName);
-    }
-    if (!success) {
-        *errorString = orig.errorString();
-        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
-        return false;
-    }
-
-#else //Q_OS_WIN
-    // You can not overwrite a read-only file on windows.
-    if (!isWritable(destinationFileName)) {
-        setFileReadOnly(destinationFileName, false);
-    }
-
-    BOOL ok = 0;
-    QString orig = longWinPath(originFileName);
-    QString dest = longWinPath(destinationFileName);
-
-    ok = MoveFileEx((wchar_t *)orig.utf16(),
-        (wchar_t *)dest.utf16(),
-        MOVEFILE_REPLACE_EXISTING + MOVEFILE_COPY_ALLOWED + MOVEFILE_WRITE_THROUGH);
-    if (!ok) {
-        *errorString = Utility::formatWinError(GetLastError());
-        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
-        return false;
-    }
-#endif
-    return true;
-}
-
 bool FileSystem::openAndSeekFileSharedRead(QFile *file, QString *errorOrNull, qint64 seek)
 {
     QString errorDummy;
@@ -598,6 +550,7 @@ bool FileSystem::remove(const QString &fileName, QString *errorString)
             qCWarning(lcFileSystem()) << "File is already deleted" << fileName;
             return false;
         }
+        qCInfo(lcFileSystem()) << "delete" << fileName;
     }
     catch (const std::filesystem::filesystem_error &e)
     {

src/common/filesystembase.h

@@ -132,14 +132,6 @@ namespace FileSystem {
         const QString &destinationFileName,
         QString *errorString = nullptr);
 
-    /**
-     * Rename the file \a originFileName to \a destinationFileName, and
-     * overwrite the destination if it already exists - without extra checks.
-     */
-    bool OCSYNC_EXPORT uncheckedRenameReplace(const QString &originFileName,
-        const QString &destinationFileName,
-        QString *errorString);
-
     /**
      * Removes a file.
      *

src/libsync/filesystem.cpp

@@ -281,6 +281,7 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
             const auto parentFolderPath = fileInfo.dir().absolutePath();
             const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
             removeOk = FileSystem::remove(di.filePath(), &removeError);
+            qCInfo(lcFileSystem()) << "delete" << di.filePath();
             if (removeOk) {
                 if (onDeleted)
                     onDeleted(di.filePath(), false);
@@ -303,7 +304,9 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
         const auto parentFolderPath = fileInfo.dir().absolutePath();
         const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
         FileSystem::setFolderPermissions(path, FileSystem::FolderPermissions::ReadWrite);
-        allRemoved = QDir().rmdir(path);
+        auto folderDeleteError = QString{};
+        allRemoved = FileSystem::remove(path, &folderDeleteError);
+        qCInfo(lcFileSystem()) << "delete" << path;
         if (allRemoved) {
             if (onDeleted)
                 onDeleted(path, true);
@@ -315,7 +318,7 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
             if (onError) {
                 onError(di.filePath(), false);
             }
-            qCWarning(lcFileSystem) << "Error removing folder" << path;
+            qCWarning(lcFileSystem) << "Error removing folder" << path << folderDeleteError;
         }
     }
     return allRemoved;
@@ -334,34 +337,6 @@ bool FileSystem::getInode(const QString &filename, quint64 *inode)
 bool FileSystem::setFolderPermissions(const QString &path,
                                       FileSystem::FolderPermissions permissions) noexcept
 {
-    static constexpr auto writePerms = std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write;
-    const auto stdStrPath = path.toStdWString();
-    try
-    {
-        switch (permissions) {
-        case OCC::FileSystem::FolderPermissions::ReadOnly:
-            std::filesystem::permissions(stdStrPath, writePerms, std::filesystem::perm_options::remove);
-            break;
-        case OCC::FileSystem::FolderPermissions::ReadWrite:
-            break;
-        }
-    }
-    catch (const std::filesystem::filesystem_error &e)
-    {
-        qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path1:" << e.path1().c_str() << "- path2:" << e.path2().c_str();
-        return false;
-    }
-    catch (const std::system_error &e)
-    {
-        qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path:" << stdStrPath;
-        return false;
-    }
-    catch (...)
-    {
-        qCWarning(lcFileSystem()) << "exception when modifying folder permissions -  path:" << stdStrPath;
-        return false;
-    }
-
 #ifdef Q_OS_WIN
     SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION;
     std::unique_ptr<char[]> securityDescriptor;
@@ -429,6 +404,10 @@ bool FileSystem::setFolderPermissions(const QString &path,
         }
     }
 
+    if (permissions == FileSystem::FolderPermissions::ReadWrite) {
+        qCInfo(lcFileSystem) << path << "will be read write";
+    }
+
     for (int i = 0; i < aclSize.AceCount; ++i) {
         void *currentAce = nullptr;
         if (!GetAce(resultDacl, i, &currentAce)) {
@@ -438,9 +417,6 @@ bool FileSystem::setFolderPermissions(const QString &path,
 
         const auto currentAceHeader = reinterpret_cast<PACE_HEADER>(currentAce);
 
-        if (permissions == FileSystem::FolderPermissions::ReadWrite) {
-            qCInfo(lcFileSystem) << path << "will be read write";
-        }
         if (permissions == FileSystem::FolderPermissions::ReadWrite && (ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) {
             qCWarning(lcFileSystem) << "AceHeader" << path << currentAceHeader->AceFlags << currentAceHeader->AceSize << currentAceHeader->AceType;
             continue;
@@ -478,7 +454,34 @@ bool FileSystem::setFolderPermissions(const QString &path,
         qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << path << GetLastError();
         return false;
     }
-#endif
+#else
+    static constexpr auto writePerms = std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write;
+    const auto stdStrPath = path.toStdWString();
+    try
+    {
+        switch (permissions) {
+        case OCC::FileSystem::FolderPermissions::ReadOnly:
+            std::filesystem::permissions(stdStrPath, writePerms, std::filesystem::perm_options::remove);
+            break;
+        case OCC::FileSystem::FolderPermissions::ReadWrite:
+            break;
+        }
+    }
+    catch (const std::filesystem::filesystem_error &e)
+    {
+        qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path1:" << e.path1().c_str() << "- path2:" << e.path2().c_str();
+        return false;
+    }
+    catch (const std::system_error &e)
+    {
+        qCWarning(lcFileSystem()) << "exception when modifying folder permissions" << e.what() << "- path:" << stdStrPath;
+        return false;
+    }
+    catch (...)
+    {
+        qCWarning(lcFileSystem()) << "exception when modifying folder permissions -  path:" << stdStrPath;
+        return false;
+    }
 
     try
     {
@@ -506,12 +509,76 @@ bool FileSystem::setFolderPermissions(const QString &path,
         qCWarning(lcFileSystem()) << "exception when modifying folder permissions -  path:" << stdStrPath;
         return false;
     }
+#endif
 
     return true;
 }
 
 bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept
 {
+#ifdef Q_OS_WIN
+    qCInfo(lcFileSystem()) << "is it read-only folder:" << QString::fromStdWString(path.wstring());
+
+    SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION;
+    std::unique_ptr<char[]> securityDescriptor;
+    auto neededLength = 0ul;
+
+    if (!GetFileSecurityW(path.wstring().c_str(), info, nullptr, 0, &neededLength)) {
+        const auto lastError = GetLastError();
+        if (lastError != ERROR_INSUFFICIENT_BUFFER) {
+            qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << lastError;
+            return false;
+        }
+
+        securityDescriptor.reset(new char[neededLength]);
+
+        if (!GetFileSecurityW(path.wstring().c_str(), info, securityDescriptor.get(), neededLength, &neededLength)) {
+            qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << GetLastError();
+            return false;
+        }
+    }
+
+    int daclPresent = false, daclDefault = false;
+    PACL resultDacl = nullptr;
+    if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, &resultDacl, &daclDefault)) {
+        qCWarning(lcFileSystem) << "error when calling GetSecurityDescriptorDacl" << path << GetLastError();
+        return false;
+    }
+    if (!daclPresent || !resultDacl) {
+        qCWarning(lcFileSystem) << "error when calling DACL needed to set a folder read-only or read-write is missing" << path;
+        return false;
+    }
+
+    PSID sid = nullptr;
+    if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sid))
+    {
+        qCWarning(lcFileSystem) << "error when calling ConvertStringSidToSidA" << path << GetLastError();
+        return false;
+    }
+
+    ACL_SIZE_INFORMATION aclSize;
+    if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) {
+        qCWarning(lcFileSystem) << "error when calling GetAclInformation" << path << GetLastError();
+        return false;
+    }
+
+    for (int i = 0; i < aclSize.AceCount; ++i) {
+        void *currentAce = nullptr;
+        if (!GetAce(resultDacl, i, &currentAce)) {
+            qCWarning(lcFileSystem) << "error when calling GetAce" << path << GetLastError();
+            return false;
+        }
+
+        const auto currentAceHeader = reinterpret_cast<PACE_HEADER>(currentAce);
+
+        if ((ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) {
+            qCInfo(lcFileSystem()) << "detected access denied ACL: assuming read-only folder:" << QString::fromStdWString(path.wstring());
+            return true;
+        }
+    }
+
+    return false;
+#else
     try
     {
         const auto folderStatus = std::filesystem::status(path);
@@ -533,6 +600,7 @@ bool FileSystem::isFolderReadOnly(const std::filesystem::path &path) noexcept
         qCWarning(lcFileSystem()) << "exception when checking folder permissions -  path:" << path;
         return false;
     }
+#endif
 }
 
 FileSystem::FilePermissionsRestore::FilePermissionsRestore(const QString &path, FolderPermissions temporaryPermissions)
@@ -568,4 +636,52 @@ FileSystem::FilePermissionsRestore::~FilePermissionsRestore()
     }
 }
 
+bool FileSystem::uncheckedRenameReplace(const QString &originFileName, const QString &destinationFileName, QString *errorString)
+{
+#ifndef Q_OS_WIN
+    bool success = false;
+    QFile orig(originFileName);
+    // We want a rename that also overwrites.  QFile::rename does not overwrite.
+    // Qt 5.1 has QSaveFile::renameOverwrite we could use.
+    // ### FIXME
+    success = true;
+    bool destExists = fileExists(destinationFileName);
+    if (destExists && !QFile::remove(destinationFileName)) {
+        *errorString = orig.errorString();
+        qCWarning(lcFileSystem) << "Target file could not be removed.";
+        success = false;
+    }
+    if (success) {
+        success = orig.rename(destinationFileName);
+    }
+    if (!success) {
+        *errorString = orig.errorString();
+        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
+        return false;
+    }
+#else //Q_OS_WIN
+    const auto originFileInfo = QFileInfo{originFileName};
+    const auto originParentFolderPath = originFileInfo.dir().absolutePath();
+    FilePermissionsRestore renameEnabler{originParentFolderPath, FileSystem::FolderPermissions::ReadWrite};
+    // You can not overwrite a read-only file on windows.
+    if (!isWritable(destinationFileName)) {
+        setFileReadOnly(destinationFileName, false);
+    }
+
+    BOOL ok = 0;
+    QString orig = longWinPath(originFileName);
+    QString dest = longWinPath(destinationFileName);
+
+    ok = MoveFileEx((wchar_t *)orig.utf16(),
+                    (wchar_t *)dest.utf16(),
+                    MOVEFILE_REPLACE_EXISTING + MOVEFILE_COPY_ALLOWED + MOVEFILE_WRITE_THROUGH);
+    if (!ok) {
+        *errorString = Utility::formatWinError(GetLastError());
+        qCWarning(lcFileSystem) << "Renaming temp file to final failed: " << *errorString;
+        return false;
+    }
+#endif
+    return true;
+}
+
 } // namespace OCC

src/libsync/filesystem.h

@@ -130,6 +130,14 @@ namespace FileSystem {
                                                   FileSystem::FolderPermissions permissions) noexcept;
 
     bool OWNCLOUDSYNC_EXPORT isFolderReadOnly(const std::filesystem::path &path) noexcept;
+
+    /**
+     * Rename the file \a originFileName to \a destinationFileName, and
+     * overwrite the destination if it already exists - without extra checks.
+     */
+    bool OWNCLOUDSYNC_EXPORT uncheckedRenameReplace(const QString &originFileName,
+                                                    const QString &destinationFileName,
+                                                    QString *errorString);
 }
 
 /** @} */

test/syncenginetestutils.cpp

@@ -51,9 +51,12 @@ void DiskFileModifier::remove(const QString &relativePath)
     if (fi.isFile()) {
         QVERIFY(_rootDir.remove(relativePath));
     } else {
-        const auto pathToDelete = fi.filePath().toStdWString();
-        std::filesystem::permissions(pathToDelete, std::filesystem::perms::owner_exec, std::filesystem::perm_options::add);
-        QVERIFY(std::filesystem::remove_all(pathToDelete));
+        const auto pathToDelete = fi.filePath();
+        const auto result = OCC::FileSystem::removeRecursively(pathToDelete);
+        if (!result) {
+            qDebug() << "delete failed for:" << pathToDelete;
+            QVERIFY(result);
+        }
     }
 }
 
@@ -70,7 +73,9 @@ void DiskFileModifier::insert(const QString &relativePath, qint64 size, char con
     file.close();
     // Set the mtime 30 seconds in the past, for some tests that need to make sure that the mtime differs.
     OCC::FileSystem::setModTime(file.fileName(), OCC::Utility::qDateTimeToTime_t(QDateTime::currentDateTimeUtc().addSecs(-30)));
-    QCOMPARE(file.size(), size);
+    if (file.size() != size) {
+        QCOMPARE(file.size(), size);
+    }
 }
 
 void DiskFileModifier::setContents(const QString &relativePath, char contentChar)
@@ -100,7 +105,11 @@ void DiskFileModifier::mkdir(const QString &relativePath)
 void DiskFileModifier::rename(const QString &from, const QString &to)
 {
     QVERIFY(_rootDir.exists(from));
-    QVERIFY(_rootDir.rename(from, to));
+    const auto result = _rootDir.rename(from, to);
+    if (!result) {
+        qDebug() << "failed to rename from:" << from << "to:" << to;
+        QVERIFY(result);
+    }
 }
 
 void DiskFileModifier::setModTime(const QString &relativePath, const QDateTime &modTime)