QFSFileEngine: make rename() on Unix not overwrite

The rename(2) system call overwrites, so instead of using it, we try to
use the link/unlink pair. This works for regular cases, but can fail if
trying to change case in case-insensitive filesystems, if we're
operating on a non-Unix filesystem (FAT) or, on Linux, if the file
doesn't belong to the calling user (BSDs permit this). For those cases,
we fall back to rename(2).

That means there's a race condition if a new file is created there. But
we at least reduce the likelihood of that happening for regular files.

Change-Id: I1eba2b016de74620bfc8fffd14ccb38fd929e5aa
Reviewed-by: David Faure <david.faure@kdab.com>

6 files changed, 73 insertions, 12 deletions

diff --git a/src/corelib/io/qfile.cpp b/src/corelib/io/qfile.cpp
index 2cfb718932..ce99ac96c0 100644
--- a/src/corelib/io/qfile.cpp
+++ b/src/corelib/io/qfile.cpp
@@ -566,18 +566,18 @@ QFile::rename(const QString &newName)
         d->setError(QFile::RenameError, tr("Source file does not exist."));
         return false;
     }
+
     // If the file exists and it is a case-changing rename ("foo" -> "Foo"),
     // compare Ids to make sure it really is a different file.
     // Note: this does not take file engines into account.
+    bool changingCase = false;
     QByteArray targetId = QFileSystemEngine::id(QFileSystemEntry(newName));
     if (!targetId.isNull()) {
         QByteArray fileId = d->fileEngine ?
                     d->fileEngine->id() :
                     QFileSystemEngine::id(QFileSystemEntry(d->fileName));
-        if (fileId != targetId || d->fileName.compare(newName, Qt::CaseInsensitive)) {
-            // ### Race condition. If a file is moved in after this, it /will/ be
-            // overwritten. On Unix, the proper solution is to use hardlinks:
-            // return ::link(old, new) && ::remove(old);
+        changingCase = (fileId == targetId && d->fileName.compare(newName, Qt::CaseInsensitive) == 0);
+        if (!changingCase) {
             d->setError(QFile::RenameError, tr("Destination file exists"));
             return false;
         }
@@ -593,7 +593,7 @@ QFile::rename(const QString &newName)
             return false;
         }
         tempFile.close();
-        if (!d->engine()->rename(tempFile.fileName())) {
+        if (!d->engine()->renameOverwrite(tempFile.fileName())) {
             d->setError(QFile::RenameError, tr("Error while renaming."));
             return false;
         }
@@ -616,7 +616,7 @@ QFile::rename(const QString &newName)
     unsetError();
     close();
     if(error() == QFile::NoError) {
-        if (d->engine()->rename(newName)) {
+        if (changingCase ? d->engine()->renameOverwrite(newName) : d->engine()->rename(newName)) {
             unsetError();
             // engine was able to handle the new name so we just reset it
             d->fileEngine->setFileName(newName);
diff --git a/src/corelib/io/qfilesystemengine_p.h b/src/corelib/io/qfilesystemengine_p.h
index e3e52f6eaa..f625f4dcd8 100644
--- a/src/corelib/io/qfilesystemengine_p.h
+++ b/src/corelib/io/qfilesystemengine_p.h
@@ -121,6 +121,7 @@ public:
 
     static bool copyFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error);
     static bool renameFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error);
+    static bool renameOverwriteFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error);
     static bool removeFile(const QFileSystemEntry &entry, QSystemError &error);
 
     static bool setPermissions(const QFileSystemEntry &entry, QFile::Permissions permissions, QSystemError &error,
diff --git a/src/corelib/io/qfilesystemengine_unix.cpp b/src/corelib/io/qfilesystemengine_unix.cpp
index b0c23e3f82..b77cea3f80 100644
--- a/src/corelib/io/qfilesystemengine_unix.cpp
+++ b/src/corelib/io/qfilesystemengine_unix.cpp
@@ -651,6 +651,50 @@ bool QFileSystemEngine::copyFile(const QFileSystemEntry &source, const QFileSyst
 //static
 bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
 {
+    QFileSystemEntry::NativePath srcPath = source.nativeFilePath();
+    QFileSystemEntry::NativePath tgtPath = target.nativeFilePath();
+    if (::link(srcPath, tgtPath) == 0) {
+        if (::unlink(srcPath) == 0)
+            return true;
+
+        // if we managed to link but can't unlink the source, it's likely
+        // it's in a directory we don't have write access to; fail the
+        // renaming instead
+        int savedErrno = errno;
+
+        // this could fail too, but there's nothing we can do about it now
+        ::unlink(tgtPath);
+
+        error = QSystemError(savedErrno, QSystemError::StandardLibraryError);
+        return false;
+    }
+
+    switch (errno) {
+    case EACCES:
+    case EEXIST:
+    case ENAMETOOLONG:
+    case ENOENT:
+    case ENOTDIR:
+    case EROFS:
+    case EXDEV:
+        // accept the error from link(2) (especially EEXIST) and don't retry
+        break;
+
+    default:
+        // fall back to rename()
+        // ### Race condition. If a file is moved in after this, it /will/ be
+        // overwritten.
+        if (::rename(srcPath, tgtPath) == 0)
+            return true;
+    }
+
+    error = QSystemError(errno, QSystemError::StandardLibraryError);
+    return false;
+}
+
+//static
+bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
+{
     if (::rename(source.nativeFilePath().constData(), target.nativeFilePath().constData()) == 0)
         return true;
     error = QSystemError(errno, QSystemError::StandardLibraryError);
diff --git a/src/corelib/io/qfilesystemengine_win.cpp b/src/corelib/io/qfilesystemengine_win.cpp
index 5a9864edb2..74c6b52a66 100644
--- a/src/corelib/io/qfilesystemengine_win.cpp
+++ b/src/corelib/io/qfilesystemengine_win.cpp
@@ -1247,6 +1247,17 @@ bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSy
 }
 
 //static
+bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
+{
+    bool ret = ::MoveFileEx(reinterpret_cast<const wchar_t *>(source.nativeFilePath().utf16()),
+                            reinterpret_cast<const wchar_t *>(target.nativeFilePath().utf16()),
+                            MOVEFILE_REPLACE_EXISTING) != 0;
+    if (!ret)
+        error = QSystemError(::GetLastError(), QSystemError::NativeError);
+    return ret;
+}
+
+//static
 bool QFileSystemEngine::removeFile(const QFileSystemEntry &entry, QSystemError &error)
 {
     bool ret = ::DeleteFile((wchar_t*)entry.nativeFilePath().utf16()) != 0;
diff --git a/src/corelib/io/qfsfileengine_unix.cpp b/src/corelib/io/qfsfileengine_unix.cpp
index ab762d2b4d..771810a06e 100644
--- a/src/corelib/io/qfsfileengine_unix.cpp
+++ b/src/corelib/io/qfsfileengine_unix.cpp
@@ -362,8 +362,14 @@ bool QFSFileEngine::copy(const QString &newName)
 
 bool QFSFileEngine::renameOverwrite(const QString &newName)
 {
-    // On Unix, rename() overwrites.
-    return rename(newName);
+    Q_D(QFSFileEngine);
+    QSystemError error;
+    bool ret = QFileSystemEngine::renameOverwriteFile(d->fileEntry, QFileSystemEntry(newName), error);
+
+    if (!ret)
+        setError(QFile::RenameError, error.toString());
+
+    return ret;
 }
 
 bool QFSFileEngine::rename(const QString &newName)
diff --git a/src/corelib/io/qfsfileengine_win.cpp b/src/corelib/io/qfsfileengine_win.cpp
index 0decd26179..023354fd8f 100644
--- a/src/corelib/io/qfsfileengine_win.cpp
+++ b/src/corelib/io/qfsfileengine_win.cpp
@@ -507,11 +507,10 @@ bool QFSFileEngine::rename(const QString &newName)
 bool QFSFileEngine::renameOverwrite(const QString &newName)
 {
     Q_D(QFSFileEngine);
-    bool ret = ::MoveFileEx((wchar_t*)d->fileEntry.nativeFilePath().utf16(),
-                            (wchar_t*)QFileSystemEntry(newName).nativeFilePath().utf16(),
-                            MOVEFILE_REPLACE_EXISTING) != 0;
+    QSystemError error;
+    bool ret = QFileSystemEngine::renameOverwriteFile(d->fileEntry, QFileSystemEntry(newName), error);
     if (!ret)
-        setError(QFile::RenameError, QSystemError(::GetLastError(), QSystemError::NativeError).toString());
+        setError(QFile::RenameError, error.toString());
     return ret;
 }