From 92373d353cf090faa03cbc8aca505d1784b10b54 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 18 Dec 2024 12:31:53 -0300 Subject: [PATCH] QSaveFile: make it so flush() errors imply commit() failed QSaveFile records past write errors in writeData(), but often the QFileDevice::writeData() calls it places will succeed because the data is only being buffered. Instead, the failures are noticed only by flush(), whose actions do not affect QSaveFilePrivate::writeError. [ChangeLog][QtCore][QSaveFile] Fixed a bug that caused commit() to return true and overwrite its intended target file even though it failed to flush buffered data to the storage, which could cause data loss. This issue can be worked around by calling flush() first and only calling commit() if that returns success. [ChangeLog][QtCore][QSaveFile] Fixed a bug that caused commit() to return true even after a cancelWriting() call had been placed, if writing directly to the target file (that is, only with setDirectWriteFallback() set to true). Note that the state of the file does not change under those conditions, only the value returned by the function. Drive-by clarify a comment from 6bf1674f1e51fd8b08783035cda7493ecd63b44 (Qt 4.6 "Don't drop errors from flush on QFile::close") which had me chasing the wrong lead. Fixes: QTBUG-132332 Pick-to: 6.9 6.8 6.5 5.15 Change-Id: I427df6fd02132d02be91fffd175579c35b9c06cc Reviewed-by: David Faure Reviewed-by: Oswald Buddenhagen --- src/corelib/io/qfiledevice.cpp | 3 +- src/corelib/io/qsavefile.cpp | 14 +- .../corelib/io/qsavefile/tst_qsavefile.cpp | 133 +++++++++++++++++- 3 files changed, 144 insertions(+), 6 deletions(-) diff --git a/src/corelib/io/qfiledevice.cpp b/src/corelib/io/qfiledevice.cpp index 431dc65f5b8..3ce75bd3237 100644 --- a/src/corelib/io/qfiledevice.cpp +++ b/src/corelib/io/qfiledevice.cpp @@ -338,7 +338,8 @@ void QFileDevice::close() // reset cached size d->cachedSize = 0; - // keep earlier error from flush + // If flush() succeeded but close() failed, copy its error condition; + // otherwise, keep the earlier flush() error. if (d->fileEngine->close() && flushed) unsetError(); else if (flushed) diff --git a/src/corelib/io/qsavefile.cpp b/src/corelib/io/qsavefile.cpp index 073992264af..b571c06c1a6 100644 --- a/src/corelib/io/qsavefile.cpp +++ b/src/corelib/io/qsavefile.cpp @@ -302,10 +302,15 @@ bool QSaveFile::commit() // Sync to disk if possible. Ignore errors (e.g. not supported). fe->syncToDisk(); + // ensure we act on either a close()/flush() failure or a previous write() + // problem + if (d->error == QFileDevice::NoError) + d->error = d->writeError; + d->writeError = QFileDevice::NoError; + if (d->useTemporaryFile) { - if (d->writeError != QFileDevice::NoError) { + if (d->error != QFileDevice::NoError) { fe->remove(); - d->writeError = QFileDevice::NoError; return false; } // atomically replace old file with new file @@ -317,7 +322,10 @@ bool QSaveFile::commit() return false; } } - return true; + + // return true if all previous write() calls succeeded and if close() and + // flush() succeeded. + return d->error == QFileDevice::NoError; } /*! diff --git a/tests/auto/corelib/io/qsavefile/tst_qsavefile.cpp b/tests/auto/corelib/io/qsavefile/tst_qsavefile.cpp index c027f8a3c11..cf9a9834bff 100644 --- a/tests/auto/corelib/io/qsavefile/tst_qsavefile.cpp +++ b/tests/auto/corelib/io/qsavefile/tst_qsavefile.cpp @@ -12,6 +12,9 @@ #include #if defined(Q_OS_UNIX) +#include +#include +#include #include // for geteuid #endif @@ -65,6 +68,15 @@ private slots: void symlink(); void directory(); +#ifdef Q_OS_UNIX + void writeFailToDevFull_data(); + void writeFailToDevFull(); +#endif +#if defined(RLIMIT_FSIZE) && (defined(Q_OS_LINUX) || defined(Q_OS_MACOS)) + void writeFailResourceLimit_data(); + void writeFailResourceLimit(); +#endif + #ifdef Q_OS_WIN void alternateDataStream_data(); void alternateDataStream(); @@ -288,8 +300,8 @@ void tst_QSaveFile::transactionalWriteNoPermissionsOnDir() QVERIFY2(file.open(QIODevice::WriteOnly), msgCannotOpen(file).constData()); QCOMPARE((int)file.error(), (int)QFile::NoError); QCOMPARE(file.write("W"), Q_INT64_C(1)); - file.cancelWriting(); // no effect, as per the documentation - QVERIFY(file.commit()); + file.cancelWriting(); + QVERIFY(!file.commit()); QVERIFY(reader.open(QIODevice::ReadOnly)); QCOMPARE(QString::fromLatin1(reader.readAll()), QString::fromLatin1("W")); @@ -525,6 +537,123 @@ void tst_QSaveFile::directory() #endif } +[[maybe_unused]] static void bufferedAndUnbuffered() +{ + QTest::addColumn("mode"); + QTest::newRow("unbuffered") << QIODevice::OpenMode(QIODevice::Unbuffered); + QTest::newRow("buffered") << QIODevice::OpenMode(); +} + +#ifdef Q_OS_UNIX +void tst_QSaveFile::writeFailToDevFull_data() +{ + // check if /dev/full exists and is writable + if (access("/dev/full", W_OK) != 0) + QSKIP("/dev/full either does not exist or is not writable"); + if (access("/dev", W_OK) == 0) + QSKIP("/dev is writable (running as root?): this test would replace /dev/full"); + + bufferedAndUnbuffered(); +} + +void tst_QSaveFile::writeFailToDevFull() +{ + QFETCH(QIODevice::OpenMode, mode); + mode |= QIODevice::WriteOnly; + + QSaveFile saveFile("/dev/full"); + saveFile.setDirectWriteFallback(true); + + QVERIFY2(saveFile.open(mode), msgCannotOpen(saveFile).constData()); + + QByteArray data("abc"); + qint64 written = saveFile.write(data); + if (mode & QIODevice::Unbuffered) { + // error reported immediately + QCOMPARE(written, -1); + QCOMPARE(saveFile.error(), QFile::ResourceError); + QCOMPARE(saveFile.errorString(), qt_error_string(ENOSPC)); + } else { + // error reported only on .commit() + QCOMPARE(written, data.size()); + QCOMPARE(saveFile.error(), QFile::NoError); + } + QVERIFY(!saveFile.commit()); + QCOMPARE(saveFile.error(), QFile::ResourceError); +} +#endif // Q_OS_UNIX +#if defined(RLIMIT_FSIZE) && (defined(Q_OS_LINUX) || defined(Q_OS_MACOS)) +// This test is only enabled on Linux and on macOS because we can verify that +// those OSes do respect RLIMIT_FSIZE. We can also verify that some other Unix +// OSes do not. + +void tst_QSaveFile::writeFailResourceLimit_data() +{ + bufferedAndUnbuffered(); +} + +void tst_QSaveFile::writeFailResourceLimit() +{ + // don't make it too small because stdout may be a log file! + static constexpr qint64 FileSizeLimit = 1024 * 1024; + struct RlimitChanger { + struct rlimit old; + RlimitChanger() + { + getrlimit(RLIMIT_FSIZE, &old); + struct rlimit newLimit = {}; + newLimit.rlim_cur = FileSizeLimit; + newLimit.rlim_max = old.rlim_max; + if (setrlimit(RLIMIT_FSIZE, &newLimit) != 0) + old.rlim_cur = 0; + + // ignore SIGXFSZ so we get EF2BIG when writing the file + signal(SIGXFSZ, SIG_IGN); + } + ~RlimitChanger() + { + if (old.rlim_cur) + setrlimit(RLIMIT_FSIZE, &old); + } + }; + + QFETCH(QIODevice::OpenMode, mode); + mode |= QIODevice::WriteOnly; + + QTemporaryDir dir; + QVERIFY2(dir.isValid(), qPrintable(dir.errorString())); + const QString targetFile = dir.path() + QString::fromLatin1("/outfile"); + QFile::remove(targetFile); + QSaveFile saveFile(targetFile); + QVERIFY2(saveFile.open(mode), msgCannotOpen(saveFile).constData()); + + RlimitChanger changer; + if (changer.old.rlim_cur == 0) + QSKIP("Could not set the file size resource limit"); + + QByteArray data(FileSizeLimit + 16, 'a'); + qint64 written = 0, lastWrite; + do { + lastWrite = saveFile.write(data.constData() + written, data.size() - written); + if (lastWrite > 0) + written += lastWrite; + } while (lastWrite > 0 && written < data.size()); + if (mode & QIODevice::Unbuffered) { + // error reported immediately + QCOMPARE_LT(written, data.size()); + QCOMPARE(lastWrite, -1); + QCOMPARE(saveFile.error(), QFile::WriteError); + QCOMPARE(saveFile.errorString(), qt_error_string(EFBIG)); + } else { + // error reported only on .commit() + QCOMPARE(written, data.size()); + QCOMPARE(saveFile.error(), QFile::NoError); + } + QVERIFY(!saveFile.commit()); + QCOMPARE(saveFile.error(), QFile::WriteError); +} +#endif // RLIMIT_FSIZE && (Linux or macOS) + #ifdef Q_OS_WIN void tst_QSaveFile::alternateDataStream_data() {