From 363dc9146e53e24172bb9c0ae68100a8543bd9ae Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 29 Jun 2017 14:03:26 -0700 Subject: [PATCH] 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 --- src/corelib/io/qfile.cpp | 12 +++---- src/corelib/io/qfilesystemengine_p.h | 1 + src/corelib/io/qfilesystemengine_unix.cpp | 44 +++++++++++++++++++++++ src/corelib/io/qfilesystemengine_win.cpp | 11 ++++++ src/corelib/io/qfsfileengine_unix.cpp | 10 ++++-- src/corelib/io/qfsfileengine_win.cpp | 7 ++-- 6 files changed, 73 insertions(+), 12 deletions(-) diff --git a/src/corelib/io/qfile.cpp b/src/corelib/io/qfile.cpp index 2cfb7189326..ce99ac96c08 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 e3e52f6eaad..f625f4dcd85 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 b0c23e3f82e..b77cea3f80c 100644 --- a/src/corelib/io/qfilesystemengine_unix.cpp +++ b/src/corelib/io/qfilesystemengine_unix.cpp @@ -650,6 +650,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; diff --git a/src/corelib/io/qfilesystemengine_win.cpp b/src/corelib/io/qfilesystemengine_win.cpp index 5a9864edb20..74c6b52a662 100644 --- a/src/corelib/io/qfilesystemengine_win.cpp +++ b/src/corelib/io/qfilesystemengine_win.cpp @@ -1246,6 +1246,17 @@ bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSy return ret; } +//static +bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error) +{ + bool ret = ::MoveFileEx(reinterpret_cast(source.nativeFilePath().utf16()), + reinterpret_cast(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) { diff --git a/src/corelib/io/qfsfileengine_unix.cpp b/src/corelib/io/qfsfileengine_unix.cpp index ab762d2b4d2..771810a06e0 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 0decd26179c..023354fd8fc 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; }