fix: leave unicode normalizations in paths untouched (#12039)

* fix: bypass Qt's QFile::encodeName() in csync

* Move decode name methods to the `OCC::FileSystem` namespace

* Use custom mkpath to ensure no normalization is done on names

* Stop the socket API from normalizing file names

* fix: implement FakeFolder::fromDisk using csync functions to bypass QFile::encodeName()

* Use `std::filesystem::rename` on non-Windows systems

To prevent `QFile::rename` doing normalization changes to the file name.

* Add file/dir name normalization test

Check that a file/directory name with NFC encoding on the server ends up
with the same encoding on the client, and that a subsequent
discovery+sync will not upload differently encoded files. Same for an
NFD encoded file/directory name.

* Fix mkpath

---------

Co-authored-by: Erik Verbruggen <erik@verbruggen.consulting>

src/common/filesystembase.cpp

@@ -26,6 +26,8 @@
 #include <QSettings>
 #include <QStorageInfo>
 
+#include <filesystem>
+
 #include <sys/stat.h>
 #include <sys/types.h>
 
@@ -49,6 +51,16 @@ namespace OCC {
 
 Q_LOGGING_CATEGORY(lcFileSystem, "sync.filesystem", QtInfoMsg)
 
+QByteArray FileSystem::encodeFileName(const QString &fileName)
+{
+    return fileName.toLocal8Bit();
+}
+
+QString FileSystem::decodeFileName(const char *localFileName)
+{
+    return QString::fromLocal8Bit(localFileName);
+}
+
 QString FileSystem::longWinPath(const QString &inpath)
 {
 #ifndef Q_OS_WIN
@@ -197,28 +209,15 @@ bool FileSystem::uncheckedRenameReplace(const QString &originFileName,
     QString *errorString)
 {
     Q_ASSERT(errorString);
+
 #ifndef Q_OS_WIN
-    bool success;
-    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();
+    std::error_code err;
+    std::filesystem::rename(originFileName.toStdString(), destinationFileName.toStdString(), err);
+    if (err) {
+        *errorString = QString::fromStdString(err.message());
         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.
 
@@ -351,6 +350,22 @@ bool FileSystem::fileExists(const QString &filename, const QFileInfo &fileInfo)
     return re;
 }
 
+bool FileSystem::mkpath(const QString &parent, const QString &newDir)
+{
+#ifdef Q_OS_WIN
+    return QDir(parent).mkpath(newDir);
+#else // POSIX
+    std::error_code err;
+    QString fullPath = parent;
+    if (!fullPath.endsWith(u'/')) {
+        fullPath += u'/';
+    }
+    fullPath += newDir;
+    std::filesystem::create_directories(fullPath.toStdString(), err);
+    return err.value() == 0;
+#endif
+}
+
 QString FileSystem::fileSystemForPath(const QString &path)
 {
     QString p = path;

src/common/filesystembase.h

@@ -47,6 +47,9 @@ OCSYNC_EXPORT Q_DECLARE_LOGGING_CATEGORY(lcFileSystem)
 namespace FileSystem {
     OCSYNC_EXPORT Q_NAMESPACE;
 
+    QByteArray OCSYNC_EXPORT encodeFileName(const QString &fileName);
+    QString OCSYNC_EXPORT decodeFileName(const char *localFileName);
+
     /**
      * List of characters not allowd in filenames on Windows
      */
@@ -105,6 +108,8 @@ namespace FileSystem {
      */
     bool OCSYNC_EXPORT fileExists(const QString &filename, const QFileInfo & = QFileInfo());
 
+    bool OCSYNC_EXPORT mkpath(const QString &parent, const QString &newDir);
+
     /**
      * @brief Rename the file \a originFileName to \a destinationFileName.
      *

src/csync/std/c_time.cpp

@@ -23,11 +23,9 @@
 
 #include "common/filesystembase.h"
 
-#include <QFile>
-
 #ifdef HAVE_UTIMES
 int c_utimes(const QString &uri, const struct timeval *times) {
-    int ret = utimes(QFile::encodeName(uri).constData(), times);
+    int ret = utimes(uri.toLocal8Bit().constData(), times);
     return ret;
 }
 #else // HAVE_UTIMES

src/csync/vio/csync_vio_local_unix.cpp

@@ -28,11 +28,11 @@
 
 #include "csync.h"
 
-#include "vio/csync_vio_local.h"
+#include "common/filesystembase.h"
 #include "common/vfs.h"
+#include "vio/csync_vio_local.h"
 
 #include <QtCore/QLoggingCategory>
-#include <QtCore/QFile>
 
 Q_LOGGING_CATEGORY(lcCSyncVIOLocal, "sync.csync.vio_local", QtInfoMsg)
 
@@ -48,7 +48,7 @@ struct csync_vio_handle_t {
 csync_vio_handle_t *csync_vio_local_opendir(const QString &name) {
     std::unique_ptr<csync_vio_handle_t> handle(new csync_vio_handle_t{});
 
-    auto dirname = QFile::encodeName(name);
+    auto dirname = OCC::FileSystem::encodeFileName(name);
 
     handle->dh = opendir(dirname.constData());
     if (!handle->dh) {
@@ -77,7 +77,7 @@ std::unique_ptr<csync_file_stat_t> csync_vio_local_readdir(csync_vio_handle_t *h
     } while (qstrcmp(dirent->d_name, ".") == 0 || qstrcmp(dirent->d_name, "..") == 0);
 
   file_stat.reset(new csync_file_stat_t);
-  file_stat->path = QFile::decodeName(dirent->d_name);
+  file_stat->path = OCC::FileSystem::decodeFileName(dirent->d_name);
 
   /* Check for availability of d_type, see manpage. */
 #if defined(_DIRENT_HAVE_D_TYPE) || defined(__APPLE__)
@@ -120,8 +120,8 @@ int csync_vio_local_stat(const QString &uri, csync_file_stat_t *buf)
 {
     struct stat sb;
 
-    if (lstat(QFile::encodeName(uri).constData(), &sb) < 0) {
-        return -1;
+    if (lstat(OCC::FileSystem::encodeFileName(uri).constData(), &sb) < 0) {
+      return -1;
     }
 
     switch (sb.st_mode & S_IFMT) {

src/gui/socketapi/socketapi.cpp

@@ -235,9 +235,7 @@ void SocketApi::slotReadSocket()
     static auto invalidListener = QSharedPointer<SocketListener>::create(nullptr);
     const auto listener = _listeners.value(socket, invalidListener);
     while (socket->canReadLine()) {
-        // Make sure to normalize the input from the socket to
-        // make sure that the path will match, especially on OS X.
-        QString line = QString::fromUtf8(socket->readLine()).normalized(QString::NormalizationForm_C);
+        QString line = QString::fromUtf8(socket->readLine());
         // Note: do NOT use QString::trimmed() here! That will also remove any trailing spaces (which _are_ part of the filename)!
         line.chop(1); // remove the '\n'
 

src/libsync/propagatorjobs.cpp

@@ -178,8 +178,8 @@ void PropagateLocalMkdir::start()
         done(SyncFileItem::NormalError, tr("Can not create local folder %1 because of a local file name clash with %2").arg(newDirStr, QDir::toNativeSeparators(clash.get())));
         return;
     }
-    QDir localDir(propagator()->localPath());
-    if (!localDir.mkpath(_item->_file)) {
+
+    if (!FileSystem::mkpath(propagator()->localPath(), _item->_file)) {
         done(SyncFileItem::NormalError, tr("could not create folder %1").arg(newDirStr));
         return;
     }

test/testfilesystem.cpp

@@ -137,6 +137,74 @@ private Q_SLOTS:
 
         QVERIFY(tmp.remove());
     }
+
+    void testMkdir_data()
+    {
+        QTest::addColumn<QString>("name");
+
+        const unsigned char a_umlaut_composed_bytes[] = {0xc3, 0xa4, 0x00};
+        const QString a_umlaut_composed = QString::fromUtf8(reinterpret_cast<const char *>(a_umlaut_composed_bytes));
+        const QString a_umlaut_decomposed = a_umlaut_composed.normalized(QString::NormalizationForm_D);
+
+        QTest::newRow("a-umlaut composed") << a_umlaut_composed;
+        QTest::newRow("a-umlaut decomposed") << a_umlaut_decomposed;
+    }
+
+    // This is not a full test, it is meant to verify that no nomalization changes are done.
+    // The implementation of `OCC::FileSystem::mkpath` relies on either Qt (Windows) or
+    // `std::filesystem` (POSIX), which should be covered by tests of their own.
+    void testMkdir()
+    {
+        QFETCH(QString, name);
+
+        auto tmp = OCC::TestUtils::createTempDir();
+        QVERIFY(OCC::FileSystem::mkpath(tmp.path(), name));
+        csync_file_stat_t buf;
+        auto dh = csync_vio_local_opendir(tmp.path());
+        QVERIFY(dh);
+        while (auto fs = csync_vio_local_readdir(dh, nullptr)) {
+            QCOMPARE(fs->path, name);
+            QCOMPARE(fs->type, ItemTypeDirectory);
+        }
+        csync_vio_local_closedir(dh);
+    }
+
+    void testRename_data()
+    {
+        QTest::addColumn<QString>("name");
+
+        const unsigned char a_umlaut_composed_bytes[] = {0xc3, 0xa4, 0x00};
+        const QString a_umlaut_composed = QString::fromUtf8(reinterpret_cast<const char *>(a_umlaut_composed_bytes));
+        const QString a_umlaut_decomposed = a_umlaut_composed.normalized(QString::NormalizationForm_D);
+
+        QTest::newRow("a-umlaut composed") << a_umlaut_composed;
+        QTest::newRow("a-umlaut decomposed") << a_umlaut_decomposed;
+    }
+
+    // This is not a full test, it is meant to verify that no nomalization changes are done.
+    void testRename()
+    {
+        QFETCH(QString, name);
+
+        auto tmp = OCC::TestUtils::createTempDir();
+        QFile f(tmp.path() + u"/abc");
+        QVERIFY(f.open(QFile::WriteOnly));
+        QByteArray data("abc");
+        QCOMPARE(f.write(data), data.size());
+        f.close();
+
+        QString err;
+        QVERIFY(OCC::FileSystem::uncheckedRenameReplace(f.fileName(), tmp.path() + u'/' + name, &err));
+
+        csync_file_stat_t buf;
+        auto dh = csync_vio_local_opendir(tmp.path());
+        QVERIFY(dh);
+        while (auto fs = csync_vio_local_readdir(dh, nullptr)) {
+            QCOMPARE(fs->path, name);
+            QCOMPARE(fs->type, ItemTypeFile);
+        }
+        csync_vio_local_closedir(dh);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestFileSystem)

test/testlocaldiscovery.cpp

@@ -238,6 +238,83 @@ private Q_SLOTS:
         QVERIFY(!fakeFolder.currentRemoteState().find(QStringLiteral("C/.foo")));
         QVERIFY(!fakeFolder.currentRemoteState().find(QStringLiteral("C/bar")));
     }
+
+    void testNameNormalization_data()
+    {
+        QTest::addColumn<QString>("correct");
+        QTest::addColumn<QString>("incorrect");
+
+        const unsigned char a_umlaut_composed_bytes[] = {0xc3, 0xa4, 0x00};
+        const QString a_umlaut_composed = QString::fromUtf8(reinterpret_cast<const char *>(a_umlaut_composed_bytes));
+        const QString a_umlaut_decomposed = a_umlaut_composed.normalized(QString::NormalizationForm_D);
+
+        QTest::newRow("a_umlaut decomposed") << a_umlaut_decomposed << a_umlaut_composed;
+        QTest::newRow("a_umlaut composed") << a_umlaut_composed << a_umlaut_decomposed;
+    }
+
+    // Test that when a file/directory name on the remote is encoded in NFC, the local name is encoded
+    // in the same way, and that a subsequent sync does not change anything. And the same for NFD.
+    void testNameNormalization()
+    {
+        QFETCH_GLOBAL(Vfs::Mode, vfsMode);
+        QFETCH_GLOBAL(bool, filesAreDehydrated);
+
+        QFETCH(QString, correct);
+        QFETCH(QString, incorrect);
+
+        // Create an empty remote folder
+        FakeFolder fakeFolder({FileInfo{}}, vfsMode, filesAreDehydrated);
+        OperationCounter counter(fakeFolder);
+
+        // Create a file with an a-umlout in the "correct" normalization:
+        fakeFolder.remoteModifier().mkdir(QStringLiteral("P"));
+        fakeFolder.remoteModifier().mkdir(QStringLiteral("P/A"));
+        fakeFolder.remoteModifier().insert(QStringLiteral("P/A/") + correct);
+
+        // Same for a directory, holding a "normal" file:
+        fakeFolder.remoteModifier().mkdir(QStringLiteral("P/B") + correct);
+        fakeFolder.remoteModifier().insert(QStringLiteral("P/B") + correct + QStringLiteral("/b"));
+
+        LocalDiscoveryTracker tracker;
+        connect(&fakeFolder.syncEngine(), &SyncEngine::itemCompleted, &tracker, &LocalDiscoveryTracker::slotItemCompleted);
+        connect(&fakeFolder.syncEngine(), &SyncEngine::finished, &tracker, &LocalDiscoveryTracker::slotSyncFinished);
+
+        // First sync: discover that there are files/directories on the server that are not yet synced to the local end
+        QVERIFY(fakeFolder.applyLocalModificationsAndSync());
+
+        // Check that locally we have the file and the directory with the correct names:
+        {
+            auto localState = fakeFolder.currentLocalState();
+            QVERIFY(localState.find(QStringLiteral("P/A/") + correct) != nullptr); // check if the file exists
+            QVERIFY(localState.find(QStringLiteral("P/B") + correct + QStringLiteral("/b")) != nullptr); // check if the file exists
+        }
+
+        counter.reset();
+
+        qDebug() << "*** MARK"; // Log marker to check if a PUT/DELETE shows up in the second sync
+
+        // Force a full local discovery on the next sync, which forces a walk of the (local) file system, reading back names (and file sizes/mtimes/etc.)...
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, {QStringLiteral("P")});
+        tracker.startSyncFullDiscovery();
+
+        // ... and start the second sync:
+        QVERIFY(fakeFolder.applyLocalModificationsAndSync());
+
+        // If the normalization of the file/directory name did not change, no rename/move/etc. should have been detected, so check that the client didn't issue
+        // any of these operations:
+        QCOMPARE(counter.nDELETE, 0);
+        QCOMPARE(counter.nMOVE, 0);
+        QCOMPARE(counter.nPUT, 0);
+
+        // Check that the remote names are unchanged, and that no "incorrect" names have been introduced:
+        FileInfo &remoteState = fakeFolder.currentRemoteState();
+        QVERIFY(remoteState.find(QStringLiteral("P/A/") + correct) != nullptr); // check if the file still exists in the original normalization
+        QVERIFY(remoteState.find(QStringLiteral("P/A/") + incorrect) == nullptr); // there should NOT be a file with another normalization
+        QVERIFY(remoteState.find(QStringLiteral("P/B") + correct + QStringLiteral("/b"))
+            != nullptr); // check if the directory still exists in the original normalization
+        QVERIFY(remoteState.find(QStringLiteral("P/B") + incorrect + QStringLiteral("/b"))
+            == nullptr); // there should NOT be a directory with another normalization
+    }
 };
 
 QTEST_GUILESS_MAIN(TestLocalDiscovery)