don't do atomic overrides with failed files

We deploy atomic renames for some files, but these renames also happen
if something about the file failed which isn't really the point of the
exercise…

Closes: 828908
(cherry picked from commit fc5db01bb7d1546944200d197866b0b5c378f100)

diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 68298d785d455e5a41f21d9b29ee1f07eee49ac4..09dce41239d301916d6125c69888526e82baa8ca 100644 (file)
--- a/apt-pkg/contrib/fileutl.cc
+++ b/apt-pkg/contrib/fileutl.cc
@@ -2619,7 +2619,7 @@ bool FileFd::Close()
    }
 
    if ((Flags & Replace) == Replace) {
    }
 
    if ((Flags & Replace) == Replace) {

-      if (rename(TemporaryFileName.c_str(), FileName.c_str()) != 0)
+      if (Failed() == false && rename(TemporaryFileName.c_str(), FileName.c_str()) != 0)

         Res &= _error->Errno("rename",_("Problem renaming the file %s to %s"), TemporaryFileName.c_str(), FileName.c_str());
 
       FileName = TemporaryFileName; // for the unlink() below.
         Res &= _error->Errno("rename",_("Problem renaming the file %s to %s"), TemporaryFileName.c_str(), FileName.c_str());
 
       FileName = TemporaryFileName; // for the unlink() below.

diff --git a/test/libapt/fileutl_test.cc b/test/libapt/fileutl_test.cc
index 607c4a195bbae2361267d96e11b1a32c6864c67e..ff13e9e9259ed39179b0f1db01e79f75e1ebeb77 100644 (file)
--- a/test/libapt/fileutl_test.cc
+++ b/test/libapt/fileutl_test.cc
@@ -317,6 +317,7 @@ TEST(FileUtlTest, flAbsPath)
 
 static void TestDevNullFileFd(unsigned int const filemode)
 {
 
 static void TestDevNullFileFd(unsigned int const filemode)
 {

+   SCOPED_TRACE(filemode);

    FileFd f("/dev/null", filemode);
    EXPECT_FALSE(f.Failed());
    EXPECT_TRUE(f.IsOpen());
    FileFd f("/dev/null", filemode);
    EXPECT_FALSE(f.Failed());
    EXPECT_TRUE(f.IsOpen());


@@ -344,3 +345,37 @@ TEST(FileUtlTest, WorkingWithDevNull)
    TestDevNullFileFd(FileFd::WriteTemp);
    TestDevNullFileFd(FileFd::WriteAtomic);
 }
    TestDevNullFileFd(FileFd::WriteTemp);
    TestDevNullFileFd(FileFd::WriteAtomic);
 }

+constexpr char const * const TESTSTRING = "This is a test";
+static void TestFailingAtomicKeepsFile(char const * const label, std::string const &filename)
+{
+   SCOPED_TRACE(label);
+   EXPECT_TRUE(FileExists(filename));
+   FileFd fd;
+   EXPECT_TRUE(fd.Open(filename, FileFd::ReadOnly));
+   char buffer[50];
+   EXPECT_NE(nullptr, fd.ReadLine(buffer, sizeof(buffer)));
+   EXPECT_STREQ(TESTSTRING, buffer);
+}
+TEST(FileUtlTest, FailingAtomic)
+{
+   FileFd fd;
+   std::string filename;
+   createTemporaryFile("failingatomic", fd, &filename, TESTSTRING);
+   TestFailingAtomicKeepsFile("init", filename);
+
+   FileFd f;
+   EXPECT_TRUE(f.Open(filename, FileFd::ReadWrite | FileFd::Atomic));
+   f.EraseOnFailure();
+   EXPECT_FALSE(f.Failed());
+   EXPECT_TRUE(f.IsOpen());
+   TestFailingAtomicKeepsFile("before-fail", filename);
+   EXPECT_TRUE(f.Write("Bad file write", 10));
+   f.OpFail();
+   EXPECT_TRUE(f.Failed());
+   TestFailingAtomicKeepsFile("after-fail", filename);
+   EXPECT_TRUE(f.Close());
+   TestFailingAtomicKeepsFile("closed", filename);
+
+   if (filename.empty() == false)
+      unlink(filename.c_str());
+}