From d932153dd781e63e456d6b62025e2fa15f3d48e9 Mon Sep 17 00:00:00 2001 From: KongQun Yang Date: Mon, 6 Aug 2018 16:09:11 -0700 Subject: [PATCH] Make MemoryFile thread-safe - Also make it explicit that MemoryFile does not support opening an already open file. An error will be returned when trying to open an already open file. Previously the code may crash with memory problems. - Also updated packager_test to use different test directories for different tests. Fixes Appveyor crash due to memory corruption: #449. Appveyor may still fail but will contain a meaningful error logging. Change-Id: Ibc9346ef7f301e416a4a09f120bca56504c939d8 --- packager/file/memory_file.cc | 101 +++++++++++++++++++------- packager/file/memory_file_unittest.cc | 2 + packager/packager_test.cc | 12 ++- 3 files changed, 85 insertions(+), 30 deletions(-) diff --git a/packager/file/memory_file.cc b/packager/file/memory_file.cc index 60a4a801a4..415a738723 100644 --- a/packager/file/memory_file.cc +++ b/packager/file/memory_file.cc @@ -11,8 +11,10 @@ #include #include #include +#include #include "packager/base/logging.h" +#include "packager/base/synchronization/lock.h" namespace shaka { namespace { @@ -23,35 +25,89 @@ class FileSystem { ~FileSystem() {} static FileSystem* Instance() { - if (!g_file_system_) - g_file_system_.reset(new FileSystem()); - - return g_file_system_.get(); + static FileSystem instance; + return &instance; } - bool Exists(const std::string& file_name) const { - return files_.find(file_name) != files_.end(); + void Delete(const std::string& file_name) { + base::AutoLock auto_lock(lock_); + + if (open_files_.find(file_name) != open_files_.end()) { + LOG(ERROR) << "File '" << file_name + << "' is still open. Deleting an open MemoryFile is not " + "allowed. Exit without deleting the file."; + return; + } + + files_.erase(file_name); } - std::vector* GetFile(const std::string& file_name) { + void DeleteAll() { + base::AutoLock auto_lock(lock_); + if (!open_files_.empty()) { + LOG(ERROR) << "There are still files open. Deleting an open MemoryFile " + "is not allowed. Exit without deleting the file."; + return; + } + files_.clear(); + } + + std::vector* Open(const std::string& file_name, + const std::string& mode) { + base::AutoLock auto_lock(lock_); + + if (open_files_.find(file_name) != open_files_.end()) { + NOTIMPLEMENTED() << "File '" << file_name + << "' is already open. MemoryFile does not support " + "open the same file before it is closed."; + return nullptr; + } + + auto iter = files_.find(file_name); + if (mode == "r") { + if (iter == files_.end()) + return nullptr; + } else if (mode == "w") { + if (iter != files_.end()) + iter->second.clear(); + } else { + NOTIMPLEMENTED() << "File mode '" << mode + << "' not supported by MemoryFile"; + return nullptr; + } + + open_files_[file_name] = mode; return &files_[file_name]; } - void Delete(const std::string& file_name) { files_.erase(file_name); } + bool Close(const std::string& file_name) { + base::AutoLock auto_lock(lock_); - void DeleteAll() { files_.clear(); } + auto iter = open_files_.find(file_name); + if (iter == open_files_.end()) { + LOG(ERROR) << "Cannot close file '" << file_name + << "' which is not open."; + return false; + } + + open_files_.erase(iter); + return true; + } private: - FileSystem() {} + FileSystem(const FileSystem&) = delete; + FileSystem& operator=(const FileSystem&) = delete; - static std::unique_ptr g_file_system_; + FileSystem() = default; - std::map > files_; - DISALLOW_COPY_AND_ASSIGN(FileSystem); + // Filename to file data map. + std::map> files_; + // Filename to file open modes map. + std::map open_files_; + + base::Lock lock_; }; -std::unique_ptr FileSystem::g_file_system_; - } // namespace MemoryFile::MemoryFile(const std::string& file_name, const std::string& mode) @@ -60,6 +116,8 @@ MemoryFile::MemoryFile(const std::string& file_name, const std::string& mode) MemoryFile::~MemoryFile() {} bool MemoryFile::Close() { + if (!FileSystem::Instance()->Close(file_name())) + return false; delete this; return true; } @@ -117,19 +175,10 @@ bool MemoryFile::Tell(uint64_t* position) { } bool MemoryFile::Open() { - FileSystem* file_system = FileSystem::Instance(); - if (mode_ == "r") { - if (!file_system->Exists(file_name())) - return false; - } else if (mode_ == "w") { - file_system->Delete(file_name()); - } else { - NOTIMPLEMENTED() << "File mode " << mode_ << " not supported by MemoryFile"; + file_ = FileSystem::Instance()->Open(file_name(), mode_); + if (!file_) return false; - } - file_ = file_system->GetFile(file_name()); - DCHECK(file_); position_ = 0; return true; } diff --git a/packager/file/memory_file_unittest.cc b/packager/file/memory_file_unittest.cc index bb44a7b5eb..f9813dd352 100644 --- a/packager/file/memory_file_unittest.cc +++ b/packager/file/memory_file_unittest.cc @@ -25,6 +25,7 @@ TEST_F(MemoryFileTest, ModifiesSameFile) { std::unique_ptr writer(File::Open("memory://file1", "w")); ASSERT_TRUE(writer); ASSERT_EQ(kWriteBufferSize, writer->Write(kWriteBuffer, kWriteBufferSize)); + writer.release()->Close(); // Since File::Open should not create a ThreadedIoFile so there should be // no cache. @@ -101,6 +102,7 @@ TEST_F(MemoryFileTest, WriteExistingFileDeletes) { std::unique_ptr file1(File::Open("memory://file1", "w")); ASSERT_TRUE(file1); ASSERT_EQ(kWriteBufferSize, file1->Write(kWriteBuffer, kWriteBufferSize)); + file1.release()->Close(); std::unique_ptr file2(File::Open("memory://file1", "w")); ASSERT_TRUE(file2); diff --git a/packager/packager_test.cc b/packager/packager_test.cc index aa239b88dc..9050388f8d 100644 --- a/packager/packager_test.cc +++ b/packager/packager_test.cc @@ -15,6 +15,7 @@ using testing::MockFunction; using testing::Return; using testing::ReturnArg; using testing::StrEq; +using testing::UnitTest; using testing::WithArgs; namespace shaka { @@ -41,8 +42,6 @@ const double kClearLeadInSeconds = 1.0; class PackagerTest : public ::testing::Test { public: - PackagerTest() {} - void SetUp() override { FILE* f = fopen(kTestFile, "rb"); if (!f) { @@ -50,6 +49,12 @@ class PackagerTest : public ::testing::Test { return; } fclose(f); + + // Use memory file for testing and generate different test directories for + // different tests. + test_directory_ = std::string("memory://test/") + + UnitTest::GetInstance()->current_test_info()->name() + + "/"; } std::string GetFullPath(const std::string& file_name) { @@ -91,8 +96,7 @@ class PackagerTest : public ::testing::Test { } protected: - // Use memory file for testing. - std::string test_directory_ = "memory://test/"; + std::string test_directory_; }; TEST_F(PackagerTest, Version) {