From 62a396b6611c8adf21d1e1dab3bc38f80ccb91ed Mon Sep 17 00:00:00 2001 From: John Preston Date: Sat, 28 Jul 2018 17:07:50 +0300 Subject: [PATCH] Fix Storage::File lock with killing and add tests. --- Telegram/SourceFiles/base/tests_main.cpp | 4 + .../storage/storage_encrypted_file.cpp | 8 +- .../storage/storage_encrypted_file_tests.cpp | 143 +++++++++++++++++- .../SourceFiles/storage/storage_file_lock.h | 2 +- .../storage/storage_file_lock_posix.cpp | 19 ++- .../storage/storage_file_lock_win.cpp | 12 +- 6 files changed, 171 insertions(+), 17 deletions(-) diff --git a/Telegram/SourceFiles/base/tests_main.cpp b/Telegram/SourceFiles/base/tests_main.cpp index dfc7fa007..fbdcb56fe 100644 --- a/Telegram/SourceFiles/base/tests_main.cpp +++ b/Telegram/SourceFiles/base/tests_main.cpp @@ -10,6 +10,8 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL #include "reporters/catch_reporter_compact.hpp" #include +int (*TestForkedMethod)()/* = nullptr*/; + namespace base { namespace assertion { @@ -84,6 +86,8 @@ int main(int argc, const char *argv[]) { for (auto i = 0; i != argc; ++i) { if (argv[i] == QString("--touch") && i + 1 != argc) { touchFile = QFile::decodeName(argv[++i]); + } else if (argv[i] == QString("--forked") && TestForkedMethod) { + return TestForkedMethod(); } } const char *catch_argv[] = { diff --git a/Telegram/SourceFiles/storage/storage_encrypted_file.cpp b/Telegram/SourceFiles/storage/storage_encrypted_file.cpp index d26cd2f8a..0b5a02c47 100644 --- a/Telegram/SourceFiles/storage/storage_encrypted_file.cpp +++ b/Telegram/SourceFiles/storage/storage_encrypted_file.cpp @@ -62,9 +62,7 @@ File::Result File::attemptOpenForRead(const EncryptionKey &key) { } File::Result File::attemptOpenForReadAppend(const EncryptionKey &key) { - if (!_data.open(QIODevice::ReadWrite)) { - return Result::Failed; - } else if (!_lock.lock(_data)) { + if (!_lock.lock(_data, QIODevice::ReadWrite)) { return Result::LockFailed; } const auto size = _data.size(); @@ -75,9 +73,7 @@ File::Result File::attemptOpenForReadAppend(const EncryptionKey &key) { } File::Result File::attemptOpenForWrite(const EncryptionKey &key) { - if (!_data.open(QIODevice::WriteOnly)) { - return Result::Failed; - } else if (!_lock.lock(_data)) { + if (!_lock.lock(_data, QIODevice::WriteOnly)) { return Result::LockFailed; } return writeHeader(key) ? Result::Success : Result::Failed; diff --git a/Telegram/SourceFiles/storage/storage_encrypted_file_tests.cpp b/Telegram/SourceFiles/storage/storage_encrypted_file_tests.cpp index 0aaa69c22..b7a5e4a19 100644 --- a/Telegram/SourceFiles/storage/storage_encrypted_file_tests.cpp +++ b/Telegram/SourceFiles/storage/storage_encrypted_file_tests.cpp @@ -9,6 +9,21 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL #include "storage/storage_encrypted_file.h" +#ifdef Q_OS_WIN +#include "platform/win/windows_dlls.h" +#endif // Q_OS_WIN + +#include + +#include +#ifdef Q_OS_MAC +#include +#elif defined Q_OS_LINUX // Q_OS_MAC +#include +#endif // Q_OS_MAC || Q_OS_LINUX + +extern int (*TestForkedMethod)(); + const auto key = Storage::EncryptionKey(bytes::make_vector( bytes::make_span("\ abcdefgh01234567abcdefgh01234567abcdefgh01234567abcdefgh01234567\ @@ -17,10 +32,56 @@ abcdefgh01234567abcdefgh01234567abcdefgh01234567abcdefgh01234567\ abcdefgh01234567abcdefgh01234567abcdefgh01234567abcdefgh01234567\ ").subspan(0, Storage::EncryptionKey::kSize))); -TEST_CASE("simple encrypted file", "[storage_encrypted_file]") { - const auto name = QString("simple.test"); - const auto test = bytes::make_span("testbytetestbyte").subspan(0, 16); +const auto name = QString("test.file"); +const auto test = bytes::make_span("testbytetestbyte").subspan(0, 16); + +struct ForkInit { + static int Method() { + Storage::File file; + const auto result = file.open( + name, + Storage::File::Mode::ReadAppend, + key); + if (result != Storage::File::Result::Success) { + return -1; + } + + auto data = bytes::vector(16); + const auto read = file.read(data); + if (read != data.size()) { + return -1; + } else if (data != bytes::make_vector(test)) { + return -1; + } + + const auto written = file.write(data); + if (written != data.size()) { + return -1; + } +#ifdef _DEBUG + while (true) { + std::this_thread::sleep_for(std::chrono::seconds(1)); + } +#else // _DEBUG + std::this_thread::sleep_for(std::chrono::seconds(1)); + return 0; +#endif // _DEBUG + } + ForkInit() { +#ifdef Q_OS_WIN + Platform::Dlls::start(); +#endif // Q_OS_WIN + + TestForkedMethod = &ForkInit::Method; + } + +}; + +ForkInit ForkInitializer; +QProcess ForkProcess; + +TEST_CASE("simple encrypted file", "[storage_encrypted_file]") { SECTION("writing file") { Storage::File file; const auto result = file.open( @@ -66,5 +127,81 @@ TEST_CASE("simple encrypted file", "[storage_encrypted_file]") { } TEST_CASE("two process encrypted file", "[storage_encrypted_file]") { + SECTION("writing file") { + Storage::File file; + const auto result = file.open( + name, + Storage::File::Mode::Write, + key); + REQUIRE(result == Storage::File::Result::Success); + + auto data = bytes::make_vector(test); + const auto written = file.write(data); + REQUIRE(written == data.size()); + } + SECTION("access from subprocess") { + SECTION("start subprocess") { + const auto application = []() -> QString { +#ifdef Q_OS_WIN + return "tests_storage.exe"; +#else // Q_OS_WIN + constexpr auto kMaxPath = 1024; + char result[kMaxPath] = { 0 }; + uint32_t size = kMaxPath; +#ifdef Q_OS_MAC + if (_NSGetExecutablePath(result, &size) == 0) { + return result; + } +#else // Q_OS_MAC + auto count = readlink("/proc/self/exe", result, size); + if (count > 0) { + return result; + } +#endif // Q_OS_MAC + return "tests_storage"; +#endif // Q_OS_WIN + }(); + + ForkProcess.start(application + " --forked"); + const auto started = ForkProcess.waitForStarted(); + REQUIRE(started); + } + SECTION("read subprocess result") { + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + + Storage::File file; + + const auto result = file.open( + name, + Storage::File::Mode::Read, + key); + REQUIRE(result == Storage::File::Result::Success); + + auto data = bytes::vector(32); + const auto read = file.read(data); + REQUIRE(read == data.size()); + REQUIRE(data == bytes::concatenate(test, test)); + } + SECTION("take subprocess result") { + REQUIRE(ForkProcess.state() == QProcess::Running); + + Storage::File file; + + const auto result = file.open( + name, + Storage::File::Mode::ReadAppend, + key); + REQUIRE(result == Storage::File::Result::Success); + + auto data = bytes::vector(32); + const auto read = file.read(data); + REQUIRE(read == data.size()); + REQUIRE(data == bytes::concatenate(test, test)); + + const auto finished = ForkProcess.waitForFinished(0); + REQUIRE(finished); + REQUIRE(ForkProcess.state() == QProcess::NotRunning); + } + } } diff --git a/Telegram/SourceFiles/storage/storage_file_lock.h b/Telegram/SourceFiles/storage/storage_file_lock.h index 48488716b..2220c11b2 100644 --- a/Telegram/SourceFiles/storage/storage_file_lock.h +++ b/Telegram/SourceFiles/storage/storage_file_lock.h @@ -17,7 +17,7 @@ class FileLock { public: FileLock(); - bool lock(const QFile &file); + bool lock(QFile &file, QIODevice::OpenMode mode); void unlock(); static constexpr auto kSkipBytes = size_type(4); diff --git a/Telegram/SourceFiles/storage/storage_file_lock_posix.cpp b/Telegram/SourceFiles/storage/storage_file_lock_posix.cpp index 5e45d9ed1..c198e4cb8 100644 --- a/Telegram/SourceFiles/storage/storage_file_lock_posix.cpp +++ b/Telegram/SourceFiles/storage/storage_file_lock_posix.cpp @@ -18,12 +18,17 @@ namespace Storage { namespace { bool KillProcess(pid_t pid) { + auto signal = SIGTERM; + auto attempts = 0; while (true) { - const auto result = kill(pid, SIGTERM); + const auto result = kill(pid, signal); if (result < 0) { return (errno == ESRCH); } usleep(10000); + if (++attempts == 50) { + signal = SIGKILL; + } } } @@ -85,8 +90,14 @@ FileLock::Lock::~Lock() { FileLock::FileLock() = default; -bool FileLock::lock(const QFile &file) { +bool FileLock::lock(QFile &file, QIODevice::OpenMode mode) { + Expects(_lock == nullptr || file.isOpen()); + unlock(); + file.close(); + if (!file.open(mode)) { + return false; + } while (true) { const auto result = Lock::Acquire(file); if (const auto descriptor = base::get_if(&result)) { @@ -94,10 +105,10 @@ bool FileLock::lock(const QFile &file) { _lock = std::make_unique(descriptor->value); return true; } - return false; + break; } else if (const auto pid = base::get_if(&result)) { if (pid->value <= 0 || !KillProcess(pid->value)) { - return false; + break; } } } diff --git a/Telegram/SourceFiles/storage/storage_file_lock_win.cpp b/Telegram/SourceFiles/storage/storage_file_lock_win.cpp index e71694746..f35e7fa3b 100644 --- a/Telegram/SourceFiles/storage/storage_file_lock_win.cpp +++ b/Telegram/SourceFiles/storage/storage_file_lock_win.cpp @@ -116,13 +116,19 @@ FileLock::Lock::~Lock() { FileLock::FileLock() = default; -bool FileLock::lock(const QFile &file) { +bool FileLock::lock(QFile &file, QIODevice::OpenMode mode) { + Expects(_lock == nullptr || file.isOpen()); + + unlock(); + file.close(); do { - unlock(); - if (const auto descriptor = Lock::Acquire(file)) { + if (!file.open(mode)) { + return false; + } else if (const auto descriptor = Lock::Acquire(file)) { _lock = std::make_unique(descriptor); return true; } + file.close(); } while (CloseProcesses(file.fileName())); return false;