do not move not-failed pdiff-patches into CWD on failure

If a single pdiff fails, we have to fail the entire patching endeavour
and fall back to getting the complete file instead. That is easy in
serverside merged pdiffs as we get them one by one. For clientside we
get them all at once through, which means that a failure in one has to
stop the entire pipeline, which works as expected (as proven by the
bugreporters as they don't even notice it happening). The problem is
just that the first failing pdiff will do the cleanup, so another pdiff
which happens to be successfully acquired after we processed the failure
doesn't find the file it is supposed to use as a basename anymore, so
the patch is renamed to what should be the unique extension and moved
into the current working directory. Processing is then stopped as the
patch realizes that it isn't the last one which completed downloading.

On the plus side this means this is neither us using a bad temporary
location nor a security problem. It "just" overrides unconditionally
files in your current working directory (if you happen to have them
named like a pdiff patch – a bit unlikely perhaps) and so drops files
there which are never used again.

I guess this was introduced in 4e3c5633b1e74b4f58b95f339cfbbf4cbf21ab3e
for real as I made the need for the existence of the base file rather
explicit, but the potential lingers in the code for far longer.

Closes: #816837

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 62d9606336ba2750da86b738f7641bd6e78ef051..ad40f8974e74541924bde8320ea19a50db02886a 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -2476,8 +2476,21 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 
    Item::Done(Message, Hashes, Cnf);
 
+   if (std::any_of(allPatches->begin(), allPatches->end(),
+           [](pkgAcqIndexMergeDiffs const * const P) { return P->State == StateErrorDiff; }))
+   {
+      if(Debug)
+        std::clog << "Another patch failed already, no point in processing this one." << std::endl;
+      return;
+   }
+
    std::string const UncompressedUnpatchedFile = GetPartialFileNameFromURI(Target.URI);
    std::string const UnpatchedFile = GetExistingFilename(UncompressedUnpatchedFile);
+   if (UnpatchedFile.empty())
+   {
+      _error->Fatal("Unpatched file %s doesn't exist (anymore)!", UnpatchedFile.c_str());
+      return;
+   }
    std::string const PatchFile = GetMergeDiffsPatchFileName(UnpatchedFile, patch.file);
    std::string const PatchedFile = GetKeepCompressedFileName(UncompressedUnpatchedFile, Target);
 

diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 253abb92cf7841519f5bcfb3be82e5fc6536730d..98d75d8947bafb34f1a10a5f6bbd859b9fdf928e 100755 (executable)
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -164,6 +164,19 @@ SHA256-Download:
        testsuccessequal "$(cat Packages-future)
 " aptcache show apt newstuff futurestuff
 
+       # we reuse the archive state of the previous test here
+       msgmsg "Testcase: downloading a patch fails, but successful fallback: $*"
+       rm -rf rootdir/var/lib/apt/lists
+       cp -a rootdir/var/lib/apt/lists-bak rootdir/var/lib/apt/lists
+       cp Packages-future aptarchive/Packages
+       rm "${PATCHFILE}.gz"
+       testsuccess apt update "$@"
+       cp rootdir/tmp/testsuccess.output patchdownload.output
+       testsuccess grep '^Falling back to normal index file acquire' patchdownload.output
+       testnopackage oldstuff
+       testsuccessequal "$(cat Packages-future)
+" aptcache show apt newstuff futurestuff
+
        msgmsg "Testcase: patch applying fails, but successful fallback: $*"
        rm -rf rootdir/var/lib/apt/lists
        cp -a rootdir/var/lib/apt/lists-bak rootdir/var/lib/apt/lists