update_engine: Fix leaking unit tests

Some of the unit tests have been leaking temp files because they don't properly unlink them. In this CL, we did some rearrangement of the ScopedTempFile class and moved it into the utils.h (instead of testing only location) so it can be used everywhere and more efficiently. Also added functionality to open an file descriptor too so users don't have to keep a different object for the file descriptor. BUG=b:162766400 TEST=cros_workon_make --board reef --test; Then looked at the /build/reef/tmp directory and no files were leaked. Change-Id: Id64a2923d30f27628120497fdefe16bf65fa3fb0 Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2500772 Tested-by: Amin Hassani <[email protected]> Reviewed-by: Jae Hoon Kim <[email protected]> Commit-Queue: Amin Hassani <[email protected]>
The-Pixel-Project · Oct 29, 2020 · ed03b44 · ed03b44
1 parent 5d56c1e
commit ed03b44
Show file tree

Hide file tree

Showing 34 changed files with 215 additions and 264 deletions.
diff --git a/common/hash_calculator_unittest.cc b/common/hash_calculator_unittest.cc
@@ -104,7 +104,7 @@ TEST_F(HashCalculatorTest, BigTest) {
 }
 
 TEST_F(HashCalculatorTest, UpdateFileSimpleTest) {
-  test_utils::ScopedTempFile data_file("data.XXXXXX");
+  ScopedTempFile data_file("data.XXXXXX");
   ASSERT_TRUE(test_utils::WriteFileString(data_file.path(), "hi"));
 
   for (const int length : {-1, 2, 10}) {
@@ -126,7 +126,7 @@ TEST_F(HashCalculatorTest, UpdateFileSimpleTest) {
 }
 
 TEST_F(HashCalculatorTest, RawHashOfFileSimpleTest) {
-  test_utils::ScopedTempFile data_file("data.XXXXXX");
+  ScopedTempFile data_file("data.XXXXXX");
   ASSERT_TRUE(test_utils::WriteFileString(data_file.path(), "hi"));
 
   for (const int length : {-1, 2, 10}) {

diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
@@ -369,7 +369,7 @@ class FileFetcherTest : public AnyHttpFetcherTest {
   HttpServer* CreateServer() override { return new NullHttpServer; }
 
  private:
-  test_utils::ScopedTempFile temp_file_{"ue_file_fetcher.XXXXXX"};
+  ScopedTempFile temp_file_{"ue_file_fetcher.XXXXXX"};
 };
 
 class MultiRangeHttpFetcherOverFileFetcherTest : public FileFetcherTest {

diff --git a/common/test_utils.h b/common/test_utils.h
@@ -138,22 +138,6 @@ class ScopedLoopbackDeviceBinder {
   DISALLOW_COPY_AND_ASSIGN(ScopedLoopbackDeviceBinder);
 };
 
-class ScopedTempFile {
- public:
-  ScopedTempFile() : ScopedTempFile("update_engine_test_temp_file.XXXXXX") {}
-
-  explicit ScopedTempFile(const std::string& pattern) {
-    EXPECT_TRUE(utils::MakeTempFile(pattern, &path_, nullptr));
-    unlinker_.reset(new ScopedPathUnlinker(path_));
-  }
-
-  const std::string& path() const { return path_; }
-
- private:
-  std::string path_;
-  std::unique_ptr<ScopedPathUnlinker> unlinker_;
-};
-
 class ScopedLoopMounter {
  public:
   explicit ScopedLoopMounter(const std::string& file_path,

diff --git a/common/utils.h b/common/utils.h
@@ -370,6 +370,42 @@ class ScopedPathUnlinker {
   DISALLOW_COPY_AND_ASSIGN(ScopedPathUnlinker);
 };
 
+class ScopedTempFile {
+ public:
+  ScopedTempFile() : ScopedTempFile("update_engine_temp.XXXXXX") {}
+
+  // If |open_fd| is true, a writable file descriptor will be opened for this
+  // file.
+  explicit ScopedTempFile(const std::string& pattern, bool open_fd = false) {
+    CHECK(utils::MakeTempFile(pattern, &path_, open_fd ? &fd_ : nullptr));
+    unlinker_.reset(new ScopedPathUnlinker(path_));
+    if (open_fd) {
+      CHECK_GE(fd_, 0);
+      fd_closer_.reset(new ScopedFdCloser(&fd_));
+    }
+  }
+  virtual ~ScopedTempFile() = default;
+
+  const std::string& path() const { return path_; }
+  int fd() const {
+    CHECK(fd_closer_);
+    return fd_;
+  }
+  void CloseFd() {
+    CHECK(fd_closer_);
+    fd_closer_.reset();
+  }
+
+ private:
+  std::string path_;
+  std::unique_ptr<ScopedPathUnlinker> unlinker_;
+
+  int fd_{-1};
+  std::unique_ptr<ScopedFdCloser> fd_closer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedTempFile);
+};
+
 // A little object to call ActionComplete on the ActionProcessor when
 // it's destructed.
 class ScopedActionCompleter {

diff --git a/common/utils_unittest.cc b/common/utils_unittest.cc
@@ -46,7 +46,7 @@ TEST(UtilsTest, WriteFileOpenFailure) {
 }
 
 TEST(UtilsTest, WriteFileReadFile) {
-  test_utils::ScopedTempFile file;
+  ScopedTempFile file;
   EXPECT_TRUE(utils::WriteFile(file.path().c_str(), "hello", 5));
 
   brillo::Blob readback;
@@ -60,7 +60,7 @@ TEST(UtilsTest, ReadFileFailure) {
 }
 
 TEST(UtilsTest, ReadFileChunk) {
-  test_utils::ScopedTempFile file;
+  ScopedTempFile file;
   brillo::Blob data;
   const size_t kSize = 1024 * 1024;
   for (size_t i = 0; i < kSize; i++) {
@@ -149,7 +149,7 @@ TEST(UtilsTest, FuzzIntTest) {
 namespace {
 void GetFileFormatTester(const string& expected,
                          const vector<uint8_t>& contents) {
-  test_utils::ScopedTempFile file;
+  ScopedTempFile file;
   ASSERT_TRUE(utils::WriteFile(file.path().c_str(),
                                reinterpret_cast<const char*>(contents.data()),
                                contents.size()));
@@ -378,7 +378,7 @@ TEST(UtilsTest, RunAsRootUnmountFilesystemFailureTest) {
 }
 
 TEST(UtilsTest, RunAsRootUnmountFilesystemBusyFailureTest) {
-  test_utils::ScopedTempFile tmp_image("img.XXXXXX");
+  ScopedTempFile tmp_image("img.XXXXXX");
 
   EXPECT_TRUE(base::CopyFile(
       test_utils::GetBuildArtifactsPath().Append("gen/disk_ext2_4k.img"),
@@ -418,7 +418,7 @@ TEST(UtilsTest, IsMountpointTest) {
   EXPECT_TRUE(mnt_dir.CreateUniqueTempDir());
   EXPECT_FALSE(utils::IsMountpoint(mnt_dir.GetPath().value()));
 
-  test_utils::ScopedTempFile file;
+  ScopedTempFile file;
   EXPECT_FALSE(utils::IsMountpoint(file.path()));
 }
 
@@ -460,7 +460,7 @@ TEST(UtilsTest, ParseDottedVersion) {
 }
 
 TEST(UtilsTest, GetFilePathTest) {
-  test_utils::ScopedTempFile file;
+  ScopedTempFile file;
   int fd = HANDLE_EINTR(open(file.path().c_str(), O_RDONLY));
   EXPECT_GE(fd, 0);
   EXPECT_EQ(file.path(), utils::GetFilePath(fd));

diff --git a/dynamic_partition_control_android_unittest.cc b/dynamic_partition_control_android_unittest.cc
@@ -34,7 +34,6 @@
 using android::dm::DmDeviceState;
 using android::snapshot::MockSnapshotManager;
 using chromeos_update_engine::test_utils::ScopedLoopbackDeviceBinder;
-using chromeos_update_engine::test_utils::ScopedTempFile;
 using std::string;
 using testing::_;
 using testing::AnyNumber;

diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
@@ -176,7 +176,7 @@ bool OmahaResponseHandlerActionTest::DoTest(const OmahaResponse& in,
 }
 
 TEST_F(OmahaResponseHandlerActionTest, SimpleTest) {
-  test_utils::ScopedTempFile test_deadline_file(
+  ScopedTempFile test_deadline_file(
       "omaha_response_handler_action_unittest-XXXXXX");
   {
     OmahaResponse in;

diff --git a/payload_consumer/bzip_extent_writer_unittest.cc b/payload_consumer/bzip_extent_writer_unittest.cc
@@ -49,7 +49,7 @@ class BzipExtentWriterTest : public ::testing::Test {
   void TearDown() override { fd_->Close(); }
 
   FileDescriptorPtr fd_;
-  test_utils::ScopedTempFile temp_file_{"BzipExtentWriterTest-file.XXXXXX"};
+  ScopedTempFile temp_file_{"BzipExtentWriterTest-file.XXXXXX"};
 };
 
 TEST_F(BzipExtentWriterTest, SimpleTest) {

diff --git a/payload_consumer/cached_file_descriptor_unittest.cc b/payload_consumer/cached_file_descriptor_unittest.cc
@@ -73,7 +73,7 @@ class CachedFileDescriptorTest : public ::testing::Test {
 
  protected:
   FileDescriptorPtr fd_{new EintrSafeFileDescriptor};
-  test_utils::ScopedTempFile temp_file_{"CachedFileDescriptor-file.XXXXXX"};
+  ScopedTempFile temp_file_{"CachedFileDescriptor-file.XXXXXX"};
   int value_{1};
   FileDescriptorPtr cfd_;
 };