From a20cf52e09cf97011ef5843bce30817971f81759 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 20 Sep 2023 22:35:56 +0200 Subject: [PATCH] QFile::moveToTrash: fix error reporting on Windows If a move-to-trash operation failed, e.g. because the file was opened by another process (or QFile), then the moveToTrash function would still return true. MSDN documents the IFileOperation::PerformOperations to return whether the operation succeeded, but evidently this is only a statement about the execution of queued up operations, not a statement about any of the operations' success. If the operation succeeded is reported by an HRESULT parameter of the IFileOperationProgressSink::PostDeleteItem implementation, and we ignored that parameter so far. Check it via the SUCCEEDED macro, and set a boolean sink variable based on that, which we can inspect to return the correct value. Augment the test case by opening those files we create ourselves, and if that fails (which it will on Windows, but not necessarily on other platforms), then try again after closing the file. If the first attempt succeeded, then the source file must also be gone. Pick-to: 6.5 6.2 5.15 Fixes: QTBUG-117383 Done-With: Thiago Macieira Change-Id: Icb82a0c9d3b337585dded622d6656e07dee33d84 Reviewed-by: Thiago Macieira Reviewed-by: Volker Hilsheimer Reviewed-by: Qt CI Bot (cherry picked from commit 6ca2008d6e1e02e85f39faab28d5c8a0c0dad082) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/io/qfilesystemengine_win.cpp | 10 ++- tests/auto/corelib/io/qfile/tst_qfile.cpp | 82 +++++++++++++++++++ .../io/qtemporaryfile/tst_qtemporaryfile.cpp | 50 +++++++++++ 3 files changed, 140 insertions(+), 2 deletions(-) diff --git a/src/corelib/io/qfilesystemengine_win.cpp b/src/corelib/io/qfilesystemengine_win.cpp index 49f375c4bb8..3abbfa3536d 100644 --- a/src/corelib/io/qfilesystemengine_win.cpp +++ b/src/corelib/io/qfilesystemengine_win.cpp @@ -840,9 +840,10 @@ public: return (dwFlags & TSF_DELETE_RECYCLE_IF_POSSIBLE) ? S_OK : E_FAIL; } HRESULT STDMETHODCALLTYPE PostDeleteItem(DWORD /* dwFlags */, IShellItem * /* psiItem */, - HRESULT /* hrDelete */, + HRESULT hrDelete, IShellItem *psiNewlyCreated) override { + deleteResult = hrDelete; if (psiNewlyCreated) { wchar_t *pszName = nullptr; psiNewlyCreated->GetDisplayName(SIGDN_FILESYSPATH, &pszName); @@ -863,6 +864,7 @@ public: HRESULT STDMETHODCALLTYPE ResumeTimer() override { return S_OK; } QString targetPath; + HRESULT deleteResult = S_OK; private: ULONG ref; }; @@ -1833,8 +1835,12 @@ bool QFileSystemEngine::moveFileToTrash(const QFileSystemEntry &source, hres = pfo->PerformOperations(); if (!SUCCEEDED(hres)) return false; - newLocation = QFileSystemEntry(sink->targetPath); + if (!SUCCEEDED(sink->deleteResult)) { + error = QSystemError(sink->deleteResult, QSystemError::NativeError); + return false; + } + newLocation = QFileSystemEntry(sink->targetPath); return true; } diff --git a/tests/auto/corelib/io/qfile/tst_qfile.cpp b/tests/auto/corelib/io/qfile/tst_qfile.cpp index 2f124a34ee2..6ea699bc24a 100644 --- a/tests/auto/corelib/io/qfile/tst_qfile.cpp +++ b/tests/auto/corelib/io/qfile/tst_qfile.cpp @@ -281,6 +281,8 @@ private slots: void moveToTrash_data(); void moveToTrash(); + void moveToTrashOpenFile_data(); + void moveToTrashOpenFile(); void stdfilesystem(); @@ -3978,6 +3980,86 @@ void tst_QFile::moveToTrash() } } +void tst_QFile::moveToTrashOpenFile_data() +{ + QTest::addColumn("useStatic"); + QTest::addColumn("success"); + + // QFile::moveToTrash() non-static member closes the file before trashing, + // so this must always succeed. + QTest::newRow("member") << false << true; + + // QFile::moveToTrash() static member cannot close the file because it + // operates on another QFile, so this operation will fail on OSes that do + // not permit deleting open files. + QTest::newRow("static") << true +#ifdef Q_OS_WIN + << false; +#else + << true; +#endif +} + +void tst_QFile::moveToTrashOpenFile() +{ +#if defined(Q_OS_ANDROID) || defined(Q_OS_WEBOS) + QSKIP("This platform doesn't implement a trash bin"); +#endif + QFETCH(bool, useStatic); + QFETCH(bool, success); + const QByteArrayView contents = "Hello, World\n"; + + QString newFileName, origFileName; + auto cleanup = qScopeGuard([&] { + if (!origFileName.isEmpty()) + QFile::remove(origFileName); + if (!newFileName.isEmpty() && newFileName != origFileName) + QFile::remove(newFileName); + }); + + origFileName = []() { + QTemporaryFile temp(QDir::homePath() + "/tst_qfile.moveToTrashOpenFile.XXXXXX"); + temp.setAutoRemove(false); + if (!temp.open()) + qWarning("Failed to create temporary file: %ls", qUtf16Printable(temp.errorString())); + return temp.fileName(); + }(); + + QFile f; + f.setFileName(origFileName); + QVERIFY2(f.open(QIODevice::ReadWrite | QIODevice::Unbuffered), qPrintable(f.errorString())); + f.write(contents.data(), contents.size()); + + QString errorString; + auto doMoveToTrash = [&](QFile *f) { + if (!f->moveToTrash()) + errorString = f->errorString(); + newFileName = f->fileName(); + }; + if (useStatic) { + // it's the same as the static QFile::moveToTrash(), but gives us + // the error string + QFile other(origFileName); + doMoveToTrash(&other); + } else { + doMoveToTrash(&f); + } + QCOMPARE_NE(f.fileName(), QString()); + + if (success) { + QCOMPARE(errorString, QString()); + QCOMPARE_NE(newFileName, origFileName); // must have changed! + QVERIFY(!QFile::exists(origFileName)); + QVERIFY(QFile::exists(newFileName)); + QCOMPARE(QFileInfo(newFileName).size(), contents.size()); + } else { + QCOMPARE_NE(errorString, QString()); + QCOMPARE(newFileName, origFileName); // mustn't have changed! + QVERIFY(QFile::exists(origFileName)); + QCOMPARE(QFileInfo(origFileName).size(), contents.size()); + } +} + void tst_QFile::stdfilesystem() { #if QT_CONFIG(cxx17_filesystem) diff --git a/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp b/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp index 556571b3462..42194fe7e5d 100644 --- a/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp +++ b/tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp @@ -63,6 +63,7 @@ private slots: void stressTest(); void rename(); void renameFdLeak(); + void moveToTrash(); void reOpenThroughQFile(); void keepOpenMode(); void resetTemplateAfterError(); @@ -621,6 +622,55 @@ void tst_QTemporaryFile::renameFdLeak() #endif } +void tst_QTemporaryFile::moveToTrash() +{ +#if defined(Q_OS_ANDROID) || defined(Q_OS_WEBOS) + QSKIP("This platform doesn't implement a trash bin"); +#endif +#ifdef Q_OS_WIN + // QTemporaryFile won't really close the file with close(), so this is + // expected to fail with a sharing violation error. + constexpr bool expectSuccess = false; +#else + constexpr bool expectSuccess = true; +#endif + const QByteArrayView contents = "Hello, World\n"; + + QTemporaryFile f(QDir::homePath() + "/tst_qtemporaryfile.moveToTrash.XXXXXX"); + QString origFileName; + auto cleanup = qScopeGuard([&] { + if (!origFileName.isEmpty()) + QFile::remove(origFileName); + if (QString fn = f.fileName(); !fn.isEmpty() && fn != origFileName) + QFile::remove(fn); + }); + + if (!f.open()) + QSKIP("Failed to create temporary file"); + f.write(contents.data(), contents.size()); + + // we need an actual file name: + // 1) so we can delete it in the clean-up guard in case we fail to trash + // 2) so that the file exists on Linux in the first place (no sense in + // trashing an unnamed file) + origFileName = f.fileName(); + + if (expectSuccess) { + QVERIFY2(f.moveToTrash(), qPrintable(f.errorString())); + QCOMPARE_NE(f.fileName(), origFileName); // must have changed! + QCOMPARE_NE(f.fileName(), QString()); + QVERIFY(!QFile::exists(origFileName)); + QVERIFY(QFile::exists(f.fileName())); + QCOMPARE(QFileInfo(f.fileName()).size(), contents.size()); + } else { + QVERIFY(!f.moveToTrash()); + QCOMPARE(f.fileName(), origFileName); // mustn't have changed! + QCOMPARE_NE(f.error(), QFile::NoError); + QCOMPARE_NE(f.errorString(), "Unknown error"); + QVERIFY(QFile::exists(origFileName)); + } +} + void tst_QTemporaryFile::reOpenThroughQFile() { QByteArray data("abcdefghij");