From d86071d783630393578046e389850953501df3ac Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 9 May 2017 12:02:12 -0700 Subject: [PATCH 1/4] Fix possible corruption of ktx cache files When creating a file like this: 1. Create file 2. Write file 3. Close file there is an opening before the file is written and closed where, if the application were to crash (or not finish for any other reason), the file would be left in an undefined state. Instead, this change updates it to do this: 1. Create temp file 2. Write temp file 3. Close temp file 4. Move temp file to final path This ensures that at step 3 we have a valid file, before we rename it. We should never end up with a final file in an undefined state, but it is possible to end up with a temp file that is an undefined state if we, for instance, crash during step 2. --- libraries/networking/src/FileCache.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 0a859d511b..0ceda7a745 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -110,10 +110,16 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { return file; } - // write the new file - FILE* saveFile = fopen(filepath.c_str(), "wb"); + // write the data to a temporary file + std::string tmpFilepath = filepath + "_tmp"; + FILE* saveFile = fopen(tmpFilepath.c_str(), "wb"); if (saveFile != nullptr && fwrite(data, metadata.length, 1, saveFile) && fclose(saveFile) == 0) { - file = addFile(std::move(metadata), filepath); + int result = rename(tmpFilepath.c_str(), filepath.c_str()); + if (result == 0) { + file = addFile(std::move(metadata), filepath); + } else { + qCWarning(file_cache, "[%s] Failed to rename %s to %s (%s)", tmpFilepath.c_str(), filepath.c_str(), strerror(errno)); + } } else { qCWarning(file_cache, "[%s] Failed to write %s (%s)", _dirname.c_str(), metadata.key.c_str(), strerror(errno)); errno = 0; From 542001b14f7e6b9ba3ab5dcac329a454c1308b0f Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 9 May 2017 14:08:43 -0700 Subject: [PATCH 2/4] Update FileCache to use QSaveFile for atomic writes --- libraries/networking/src/FileCache.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 0ceda7a745..348206a863 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -17,6 +17,7 @@ #include #include +#include #include @@ -110,19 +111,13 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { return file; } - // write the data to a temporary file - std::string tmpFilepath = filepath + "_tmp"; - FILE* saveFile = fopen(tmpFilepath.c_str(), "wb"); - if (saveFile != nullptr && fwrite(data, metadata.length, 1, saveFile) && fclose(saveFile) == 0) { - int result = rename(tmpFilepath.c_str(), filepath.c_str()); - if (result == 0) { - file = addFile(std::move(metadata), filepath); - } else { - qCWarning(file_cache, "[%s] Failed to rename %s to %s (%s)", tmpFilepath.c_str(), filepath.c_str(), strerror(errno)); - } + QSaveFile saveFile(QString::fromStdString(filepath)); + saveFile.open(QIODevice::WriteOnly); + saveFile.write(data, metadata.length); + if (saveFile.commit()) { + file = addFile(std::move(metadata), filepath); } else { - qCWarning(file_cache, "[%s] Failed to write %s (%s)", _dirname.c_str(), metadata.key.c_str(), strerror(errno)); - errno = 0; + qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str()); } return file; From 6e4c5d1ab4bb336d9fbd386602a48b51e221796e Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 9 May 2017 16:27:02 -0700 Subject: [PATCH 3/4] Update FileCache writing to check QSaveFile::write return value --- libraries/networking/src/FileCache.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 348206a863..0f90807d08 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -112,9 +112,8 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { } QSaveFile saveFile(QString::fromStdString(filepath)); - saveFile.open(QIODevice::WriteOnly); - saveFile.write(data, metadata.length); - if (saveFile.commit()) { + if (saveFile.open(QIODevice::WriteOnly) && saveFile.write(data, metadata.length) == metadata.length + && saveFile.commit()) { file = addFile(std::move(metadata), filepath); } else { qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str()); From d92bdbf0ed5c5a33a9a75b40ad536465b1e6fba1 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 10 May 2017 09:43:55 -0700 Subject: [PATCH 4/4] Fix unsigned/signed comparision warning --- libraries/networking/src/FileCache.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 0f90807d08..43055e7ed6 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -112,8 +112,10 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { } QSaveFile saveFile(QString::fromStdString(filepath)); - if (saveFile.open(QIODevice::WriteOnly) && saveFile.write(data, metadata.length) == metadata.length + if (saveFile.open(QIODevice::WriteOnly) + && saveFile.write(data, metadata.length) == static_cast(metadata.length) && saveFile.commit()) { + file = addFile(std::move(metadata), filepath); } else { qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str());