From 370e024224b8116840173201d51c0c2a170e0e93 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 24 Apr 2022 00:24:40 +0200 Subject: Alternatively calculate alternative file hashes in file method If we do not have the requested file we haven't calculated the hashes for it either and it is likely that the alternative file will be used, so to save the main thread from being busy with calculating hashes we do the calculation here in the method. --- methods/file.cc | 2 ++ 1 file changed, 2 insertions(+) (limited to 'methods/file.cc') diff --git a/methods/file.cc b/methods/file.cc index b2fe133f2..ff93f83d0 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -107,6 +107,8 @@ bool FileMethod::Fetch(FetchItem *Itm) AltRes.IMSHit = false; if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) AltRes.IMSHit = true; + if (Res.Filename.empty()) + CalculateHashes(Itm, AltRes); break; } // no break here as we could have situations similar to '.gz' vs '.tar.gz' here -- cgit v1.2.3-70-g09d2 From 7adf8c1fa8d519b8b57292763eb7703e7d3cc580 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 9 May 2022 11:42:48 +0200 Subject: Look at non by-hash paths in copy and file methods Ideally copy and file mirrors would support by-hash as well, but its harder to setup and maintain especially if you want to cache an online mirror who has by-hash enabled. We can avoid unneeded roundtrips (in the best case) and entire cache misses (in the usual worst case) by "just" telling methods that the URI we passed it has the requested file perhaps also in other paths. This is done in pseudo-relative paths as we would otherwise need to teach redirection code to rewrite those URIs as well. A method like http can easily ignore this value and await explicit instructions to look at that file, but inspecting the path in local sources via file or copy is (comparatively) free, so we just do it immediately. If that ends up being the wrong version of the file as by-hash would have protected us from we are in this feature branch now falling back to other mirrors which are like the ones online and in support of by-hash. --- apt-pkg/acquire-item.cc | 11 +- methods/aptmethod.h | 11 ++ methods/copy.cc | 90 ++++++++++------- methods/file.cc | 111 ++++++++++++--------- test/integration/test-apt-by-hash-update | 8 +- .../integration/test-apt-get-update-unauth-warning | 8 +- .../test-apt-update-incomplete-file-mirror | 5 + test/integration/test-method-mirror | 1 - 8 files changed, 143 insertions(+), 102 deletions(-) (limited to 'methods/file.cc') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 2c3f45076..3950ae030 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -473,15 +473,16 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item) // now add the actual by-hash uris auto const Expected = GetExpectedHashes(); auto const TargetHash = Expected.find(nullptr); - auto const PushByHashURI = [&](std::string U) { + auto const PushByHashURI = [&](std::string const &U) { if (unlikely(TargetHash == nullptr)) return false; - auto const trailing_slash = U.find_last_of("/"); + ::URI uri{U}; + auto const trailing_slash = uri.Path.find_last_of("/"); if (unlikely(trailing_slash == std::string::npos)) return false; - auto byhashSuffix = "/by-hash/" + TargetHash->HashType() + "/" + TargetHash->HashValue(); - U.replace(trailing_slash, U.length() - trailing_slash, std::move(byhashSuffix)); - PushAlternativeURI(std::move(U), {}, false); + auto altPath = uri.Path.substr(0, trailing_slash) + "/by-hash/" + TargetHash->HashType() + "/" + TargetHash->HashValue(); + std::swap(uri.Path, altPath); + PushAlternativeURI(uri, {{"Alternate-Paths", "../../" + flNotDir(altPath)}}, false); return true; }; PushByHashURI(Item.URI); diff --git a/methods/aptmethod.h b/methods/aptmethod.h index 1c24f3a98..1ea173075 100644 --- a/methods/aptmethod.h +++ b/methods/aptmethod.h @@ -514,6 +514,17 @@ protected: return part; } + static std::string CombineWithAlternatePath(std::string Path, std::string Change) + { + while (APT::String::Startswith(Change, "../")) + { + Path.erase(Path.find_last_not_of('/')); + Path = flNotFile(Path); + Change.erase(0, 3); + } + return flCombine(Path, Change); + } + aptMethod(std::string &&Binary, char const *const Ver, unsigned long const Flags) APT_NONNULL(3) : pkgAcqMethod(Ver, Flags), aptConfigWrapperForMethods(Binary), Binary(std::move(Binary)), SeccompFlags(0) { diff --git a/methods/copy.cc b/methods/copy.cc index 82eed150c..b32131a21 100644 --- a/methods/copy.cc +++ b/methods/copy.cc @@ -26,7 +26,7 @@ class CopyMethod : public aptMethod { - virtual bool Fetch(FetchItem *Itm) APT_OVERRIDE; + bool URIAcquire(std::string const &Message, FetchItem *Itm) APT_OVERRIDE; public: CopyMethod() : aptMethod("copy", "1.0", SingleInstance | SendConfig | SendURIEncoded) @@ -36,55 +36,71 @@ class CopyMethod : public aptMethod }; // CopyMethod::Fetch - Fetch a file /*{{{*/ -// --------------------------------------------------------------------- -/* */ -bool CopyMethod::Fetch(FetchItem *Itm) +bool CopyMethod::URIAcquire(std::string const &Message, FetchItem *Itm) { - // this ensures that relative paths work in copy - std::string const File = DecodeSendURI(Itm->Uri.substr(Itm->Uri.find(':')+1)); - - // Stat the file and send a start message - struct stat Buf; - if (stat(File.c_str(),&Buf) != 0) - return _error->Errno("stat",_("Failed to stat")); + struct FileCopyType { + std::string name; + struct stat stat{}; + explicit FileCopyType(std::string &&file) : name{std::move(file)} {} + }; + std::vector files; + // this ensures that relative paths work + files.emplace_back(DecodeSendURI(Itm->Uri.substr(Itm->Uri.find(':')+1))); + for (auto const &AltPath : VectorizeString(LookupTag(Message, "Alternate-Paths"), '\n')) + files.emplace_back(CombineWithAlternatePath(flNotFile(files[0].name), DecodeSendURI(AltPath))); + files.erase(std::remove_if(files.begin(), files.end(), [](auto &file) + { return stat(file.name.c_str(), &file.stat) != 0; }), + files.end()); + if (files.empty()) + return _error->Errno("copy-stat", _("Failed to stat")); // Forumulate a result and send a start message FetchResult Res; - Res.Size = Buf.st_size; Res.Filename = Itm->DestFile; - Res.LastModified = Buf.st_mtime; Res.IMSHit = false; - URIStart(Res); - // just calc the hashes if the source and destination are identical - if (File == Itm->DestFile || Itm->DestFile == "/dev/null") + for (auto const &File : files) { + Res.Size = File.stat.st_size; + Res.LastModified = File.stat.st_mtime; + + // just calc the hashes if the source and destination are identical + if (Itm->DestFile == "/dev/null" || File.name == Itm->DestFile) + { + URIStart(Res); + CalculateHashes(Itm, Res); + URIDone(Res); + return true; + } + + FileFd From(File.name, FileFd::ReadOnly); + FileFd To(Itm->DestFile, FileFd::WriteAtomic); + To.EraseOnFailure(); + if (not From.IsOpen() || not To.IsOpen()) + continue; + + // Copy the file + URIStart(Res); + if (not CopyFile(From, To)) + { + To.OpFail(); + continue; + } + From.Close(); + To.Close(); + CalculateHashes(Itm, Res); - URIDone(Res); - return true; - } + if (not Itm->ExpectedHashes.empty() && Itm->ExpectedHashes != Res.Hashes) + continue; - // See if the file exists - FileFd From(File,FileFd::ReadOnly); - FileFd To(Itm->DestFile,FileFd::WriteAtomic); - To.EraseOnFailure(); + if (not TransferModificationTimes(File.name.c_str(), Res.Filename.c_str(), Res.LastModified)) + continue; - // Copy the file - if (CopyFile(From,To) == false) - { - To.OpFail(); - return false; + URIDone(Res); + return true; } - From.Close(); - To.Close(); - - if (TransferModificationTimes(File.c_str(), Res.Filename.c_str(), Res.LastModified) == false) - return false; - - CalculateHashes(Itm, Res); - URIDone(Res); - return true; + return false; } /*}}}*/ diff --git a/methods/file.cc b/methods/file.cc index ff93f83d0..279711195 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -29,7 +29,7 @@ class FileMethod : public aptMethod { - virtual bool Fetch(FetchItem *Itm) APT_OVERRIDE; + bool URIAcquire(std::string const &Message, FetchItem *Itm) APT_OVERRIDE; public: FileMethod() : aptMethod("file", "1.0", SingleInstance | SendConfig | LocalOnly | SendURIEncoded) @@ -41,88 +41,101 @@ class FileMethod : public aptMethod // FileMethod::Fetch - Fetch a file /*{{{*/ // --------------------------------------------------------------------- /* */ -bool FileMethod::Fetch(FetchItem *Itm) +bool FileMethod::URIAcquire(std::string const &Message, FetchItem *Itm) { URI Get(Itm->Uri); - std::string const File = DecodeSendURI(Get.Path); - FetchResult Res; if (Get.Host.empty() == false) return _error->Error(_("Invalid URI, local URIS must not start with //")); - struct stat Buf; + std::vector Files; + Files.emplace_back(DecodeSendURI(Get.Path)); + for (auto const &AltPath : VectorizeString(LookupTag(Message, "Alternate-Paths"), '\n')) + Files.emplace_back(CombineWithAlternatePath(flNotFile(Files[0]), DecodeSendURI(AltPath))); + + FetchResult Res; // deal with destination files which might linger around - if (lstat(Itm->DestFile.c_str(), &Buf) == 0) + struct stat Buf; + if (lstat(Itm->DestFile.c_str(), &Buf) == 0 && S_ISLNK(Buf.st_mode) && Buf.st_size > 0) + { + char name[Buf.st_size + 1]; + if (ssize_t const sp = readlink(Itm->DestFile.c_str(), name, Buf.st_size); sp == -1) + { + Itm->LastModified = 0; + RemoveFile("file", Itm->DestFile); + } + } + + int olderrno = 0; + // See if the file exists + for (auto const &File : Files) { - if ((Buf.st_mode & S_IFREG) != 0) + if (stat(File.c_str(), &Buf) == 0) { + Res.Size = Buf.st_size; + Res.Filename = File; + Res.LastModified = Buf.st_mtime; + Res.IMSHit = false; if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) { - if (Itm->ExpectedHashes.VerifyFile(File)) - { - Res.Filename = Itm->DestFile; + auto const filesize = Itm->ExpectedHashes.FileSize(); + if (filesize != 0 && filesize == Res.Size) Res.IMSHit = true; - } } + break; } + if (olderrno == 0) + olderrno = errno; } - if (Res.IMSHit != true) - RemoveFile("file", Itm->DestFile); - int olderrno = 0; - // See if the file exists - if (stat(File.c_str(),&Buf) == 0) + if (not Res.IMSHit) { - Res.Size = Buf.st_size; - Res.Filename = File; - Res.LastModified = Buf.st_mtime; - Res.IMSHit = false; - if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) + RemoveFile("file", Itm->DestFile); + if (not Res.Filename.empty()) { - unsigned long long const filesize = Itm->ExpectedHashes.FileSize(); - if (filesize != 0 && filesize == Res.Size) - Res.IMSHit = true; + URIStart(Res); + CalculateHashes(Itm, Res); } - - CalculateHashes(Itm, Res); } - else - olderrno = errno; - if (Res.IMSHit == false) - URIStart(Res); // See if the uncompressed file exists and reuse it FetchResult AltRes; - AltRes.Filename.clear(); - std::vector extensions = APT::Configuration::getCompressorExtensions(); - for (std::vector::const_iterator ext = extensions.begin(); ext != extensions.end(); ++ext) + for (auto const &File : Files) { - if (APT::String::Endswith(File, *ext) == true) + for (const auto &ext : APT::Configuration::getCompressorExtensions()) { - std::string const unfile = File.substr(0, File.length() - ext->length()); - if (stat(unfile.c_str(),&Buf) == 0) + if (APT::String::Endswith(File, ext)) { - AltRes.Size = Buf.st_size; - AltRes.Filename = unfile; - AltRes.LastModified = Buf.st_mtime; - AltRes.IMSHit = false; - if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) - AltRes.IMSHit = true; - if (Res.Filename.empty()) - CalculateHashes(Itm, AltRes); - break; + std::string const unfile = File.substr(0, File.length() - ext.length()); + if (stat(unfile.c_str(), &Buf) == 0) + { + AltRes.Size = Buf.st_size; + AltRes.Filename = unfile; + AltRes.LastModified = Buf.st_mtime; + AltRes.IMSHit = false; + if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) + AltRes.IMSHit = true; + if (Res.Filename.empty()) + { + URIStart(Res); + CalculateHashes(Itm, AltRes); + } + break; + } + // no break here as we could have situations similar to '.gz' vs '.tar.gz' here } - // no break here as we could have situations similar to '.gz' vs '.tar.gz' here } + if (not AltRes.Filename.empty()) + break; } - if (AltRes.Filename.empty() == false) + if (not AltRes.Filename.empty()) URIDone(Res,&AltRes); - else if (Res.Filename.empty() == false) + else if (not Res.Filename.empty()) URIDone(Res); else { errno = olderrno; - return _error->Errno(File.c_str(), _("File not found")); + return _error->Errno(Files[0].c_str(), _("File not found")); } return true; diff --git a/test/integration/test-apt-by-hash-update b/test/integration/test-apt-by-hash-update index 65c3766d0..a33eb9117 100755 --- a/test/integration/test-apt-by-hash-update +++ b/test/integration/test-apt-by-hash-update @@ -13,6 +13,7 @@ insertpackage 'unstable' 'foo' 'all' '1.0' insertpackage 'unstable' 'bar' 'i386' '1.0' setupaptarchive --no-update +changetowebserver # make Packages *only* accessible by-hash for this test makebyhashonly() { @@ -100,8 +101,7 @@ msgmsg 'Test InRelease by-hash with' 'no fallback' rm -rf aptarchive/dists cp -a aptarchive/dists.bak aptarchive/dists -testfailureequal "Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease -Err:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease - File not found - ${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/by-hash/SHA256/${inrelease_hash} (2: No such file or directory) +testfailureequal "Err:1 http://localhost:${APTHTTPPORT} unstable InRelease + 404 Not Found Reading package lists... -E: Failed to fetch file:${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/InRelease File not found - ${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/by-hash/SHA256/${inrelease_hash} (2: No such file or directory)" aptget update +E: Failed to fetch http://localhost:${APTHTTPPORT}/dists/unstable/InRelease 404 Not Found" aptget update diff --git a/test/integration/test-apt-get-update-unauth-warning b/test/integration/test-apt-get-update-unauth-warning index 46df14c29..9f252ad9d 100755 --- a/test/integration/test-apt-get-update-unauth-warning +++ b/test/integration/test-apt-get-update-unauth-warning @@ -28,9 +28,7 @@ testwarning aptget update --allow-insecure-repositories rm -rf rootdir/var/lib/apt/lists find "$APTARCHIVE/dists/unstable" -name '*Release*' -delete # update without authenticated files leads to warning -testfailureequal "Get:1 file:$APTARCHIVE unstable InRelease -Ign:1 file:$APTARCHIVE unstable InRelease -Get:2 file:$APTARCHIVE unstable Release +testfailureequal "Ign:1 file:$APTARCHIVE unstable InRelease Err:2 file:$APTARCHIVE unstable Release File not found - ${APTARCHIVE}/dists/unstable/Release (2: No such file or directory) Reading package lists... @@ -45,9 +43,7 @@ lock partial' ls rootdir/var/lib/apt/lists # allow override -testwarningequal "Get:1 file:$APTARCHIVE unstable InRelease -Ign:1 file:$APTARCHIVE unstable InRelease -Get:2 file:$APTARCHIVE unstable Release +testwarningequal "Ign:1 file:$APTARCHIVE unstable InRelease Ign:2 file:$APTARCHIVE unstable Release Get:3 file:$APTARCHIVE unstable/main Sources Get:4 file:$APTARCHIVE unstable/main i386 Packages diff --git a/test/integration/test-apt-update-incomplete-file-mirror b/test/integration/test-apt-update-incomplete-file-mirror index 959ddc1a6..dec9383c9 100755 --- a/test/integration/test-apt-update-incomplete-file-mirror +++ b/test/integration/test-apt-update-incomplete-file-mirror @@ -37,6 +37,7 @@ testfailure apt update -o Debug::pkgAcquire::Worker=1 testsuccessequal '4' grep -c -- '- Filesize:' rootdir/tmp/testfailure.output testsuccessequal '2' grep -c '%0aAlt-Checksum-FileSize-Hash:%20' rootdir/tmp/testfailure.output +echo 'Acquire::By-Hash "force";' > rootdir/etc/apt/apt.conf.d/99force-by-hash.conf msgmsg 'Fallback over hashsum errors' rm -f rootdir/etc/apt/sources.list rootdir/etc/apt/sources.list.d/* @@ -47,6 +48,8 @@ copy:${TMPWORKINGDIRECTORY}/aptarchive priority:1 file:${TMPWORKINGDIRECTORY}/aptarchive2 priority:2 EOF testsuccess apt update +cp -a rootdir/tmp/testsuccess.output aptupdate.log +testsuccessequal '2' grep -c -- '^Ign:' aptupdate.log rm -rf rootdir/var/lib/apt/lists cat > mirror.list < aptarchive/mirror.txt testsuccessequal "Get:1 foo+file:${APTARCHIVE}/mirror.txt Mirrorlist [$(stat -c%s 'aptarchive/mirror.txt') B] -Get:2 foo+file:/nonexistent/apt/archive unstable InRelease Ign:2 foo+file:/nonexistent/apt/archive unstable InRelease File not found - /nonexistent/apt/archive/dists/unstable/InRelease (2: No such file or directory) Hit:2 foo+http://localhost:${APTHTTPPORT}/redirectme unstable InRelease -- cgit v1.2.3-70-g09d2