Control: retitle -1 unlock: apt/2.2.4 Control: tag -1 - moreinfo On Sat, Jun 05, 2021 at 07:46:19PM +0200, Julian Andres Klode wrote: > On Tue, Jun 01, 2021 at 10:09:44PM +0200, Paul Gevers wrote: > > Control: tags -1 moreinfo > > > > Hi Julian, > > > > Sorry it took so long to reply; pre-approvals are regularly awkward. > > > > On Mon, 17 May 2021 17:07:08 +0200 Julian Andres Klode <j...@debian.org> > > wrote: > > > Please unblock package apt > > > > Can you elaborate how severe do you think these issues are? I mean, I > > guess you were in doubt if they qualify for the freeze policy (typically > > if the maintainer doubts, the update doesn't qualify). Or were there > > different reasons why you didn't just upload and ask for a regular unblock? > > My understanding is that release team prefers pre-approvals for more > complex uploads in key packages where it's not just one RC bug being > fixed or well "it's how we always do it, except for emergency hotfixes". > > > > > To me it seems: > > We also found out that the JSON hooks are being installed by snapd, so > people with that installed might actually see bugs, though we haven't > seen them before (not sure why it works :D). > > > * The EOF could be a real thing, but the bug was reported by you and > > only found during testing. Is this a regression or has it long been there? > > It's been there forever, but it's not triggered in testing before for > unknown reasons. It's making it harder for me to > > > * TLS handshake is nice to have (for consistency). > > It's vital to ensure people get sensible re > > > * phased policy isn't a thing in Debian, so not relevant AFAICT > > Not at the moment, but the fix doesn't hurt us either, and would allow > us (or people deploying Debian w/ custom repos) to make use of it in the > future. > > > > > I'm tempted to NACK. > > It seems we have another more important bug in file quoting reported on > IRC today that can break downloads from repos that used to work if they > contained "special" characters: > https://salsa.debian.org/apt-team/apt/-/merge_requests/175 > > I've not looked into it much yet. > > Would you be tempted to NACK those small bug fixes (they are after all, > except for JSON, all single or two line logic or return value fixes) if > they accompanied the more critical bug fix? > > > Also not sure if we want all of those 3 commits or just the first > one. I can look into it in detail on Monday.
I had not received a reply, so I went ahead, included the first of those changes into the existing 2.2.4 and uploaded it to unstable as the URL quoting regression made this release critical. In case you are not aware of the background: We used to have all URLs in the acquire system decoded and then quoted them when doing GET; which caused bugs - we had to dequote URLs we got for redirects, and then the requoting quoted stuff differently, so files were not found. This was fixed in 2.1.15, but we missed 2 out of 3 places where the URL is built for .*deb files. The ones most used even :/ Scary set of changes :) This means that ugh, if the package name includes epochs, it will fail to download. Not a problem for dak repos, but um, apparently there are repos out there that do include them... Let's have a look at the code parts of the diff, here in the form of the git diff, as filtering that was a bit easier - I left out the doc typo fix, and the large swath of improved and added tests. Full debdiff is attached. I'll start by looking at the acquire quoting changes. I'm not sure how this bit plays into the Filename field specifically (because pkgAcqArchive is not a subclass of pkgAcqFile), but DonKult included it in the commit, so there must be a reason for it. Certainly it was wrong. Original state: URLs used to be unquoted, so local filenames were unquoted two In 2.1.15-2.2.3: URLs can be passed in quoted or unquoted, and will be quoted if spaces are in there (or no %). That optional quoting happened after determining the destination filename, so filenames were still unquoted - unless you happen to call with a quoted filename already in which case, ugh bad. So, super inconsistent. For 2.2.4: Filename is always quoted at the start if not quoted yet, and then dequoted for local destination filename, ensuring that we download to the same filename whether you pass in the name pre-quoted or it's quoted on demand. diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 90bcc50fa..73d2b0c8a 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -3852,16 +3852,16 @@ pkgAcqFile::pkgAcqFile(pkgAcquire *const Owner, string const &URI, HashStringLis const string &DestDir, const string &DestFilename, bool const IsIndexFile) : Item(Owner), d(NULL), IsIndexFile(IsIndexFile), ExpectedHashes(Hashes) { + ::URI url{URI}; + if (url.Path.find(' ') != std::string::npos || url.Path.find('%') == std::string::npos) + url.Path = pkgAcquire::URIEncode(url.Path); + if(!DestFilename.empty()) DestFile = DestFilename; else if(!DestDir.empty()) - DestFile = DestDir + "/" + flNotDir(URI); + DestFile = DestDir + "/" + DeQuoteString(flNotDir(url.Path)); else - DestFile = flNotDir(URI); - - ::URI url{URI}; - if (url.Path.find(' ') != std::string::npos || url.Path.find('%') == std::string::npos) - url.Path = pkgAcquire::URIEncode(url.Path); + DestFile = DeQuoteString(flNotDir(url.Path)); // Create the item Desc.URI = std::string(url); The indexfile.cc changes are trivial and quote the Filename field when appending it to the URI: diff --git a/apt-pkg/indexfile.cc b/apt-pkg/indexfile.cc index ef3486ab8..f13b52940 100644 --- a/apt-pkg/indexfile.cc +++ b/apt-pkg/indexfile.cc @@ -9,6 +9,7 @@ // Include Files /*{{{*/ #include <config.h> +#include <apt-pkg/acquire.h> #include <apt-pkg/aptconfiguration.h> #include <apt-pkg/configuration.h> #include <apt-pkg/deblistparser.h> @@ -174,7 +175,7 @@ pkgDebianIndexTargetFile::pkgDebianIndexTargetFile(IndexTarget const &Target, bo /*}}}*/ std::string pkgDebianIndexTargetFile::ArchiveURI(std::string const &File) const/*{{{*/ { - return Target.Option(IndexTarget::REPO_URI) + File; + return Target.Option(IndexTarget::REPO_URI) + pkgAcquire::URIEncode(File); } /*}}}*/ std::string pkgDebianIndexTargetFile::Describe(bool const Short) const /*{{{*/ @@ -281,7 +282,7 @@ std::string pkgDebianIndexRealFile::Describe(bool const /*Short*/) const/*{{{*/ /*}}}*/ std::string pkgDebianIndexRealFile::ArchiveURI(std::string const &/*File*/) const/*{{{*/ { - return "file:" + File; + return "file:" + pkgAcquire::URIEncode(File); } /*}}}*/ std::string pkgDebianIndexRealFile::IndexFileName() const /*{{{*/ I believe as mentioned before, that the remaining issues are "important"; and that those fixes are either isolated to the JSON hooks (only relevant if you run JSON hooks, e.g. if snapd is installed); or trivial, so the regression potential is minimal. The next change in policy handles the phased update stuff, which is not super relevant for us right now, but we could make use of it if we figure out we need to. e.g. complex bootloader security updates could be rolled out slower or whatever. It's a single word. diff --git a/apt-pkg/policy.cc b/apt-pkg/policy.cc index 39879f754..1179f422d 100644 --- a/apt-pkg/policy.cc +++ b/apt-pkg/policy.cc @@ -310,7 +310,7 @@ static inline bool ExcludePhased(std::string machineID, pkgCache::VerIterator co } APT_PURE signed short pkgPolicy::GetPriority(pkgCache::VerIterator const &Ver, bool ConsiderFiles) { - if (Ver.ParentPkg()->CurrentVer && Ver.PhasedUpdatePercentage() != 100) + if (Ver.PhasedUpdatePercentage() != 100) { if (ExcludePhased(d->machineID, Ver)) return 1; The media change issue I probably need to actually fix the underlying bug, but the hang when stdin is /dev/null is also hanging up the autopkgtests when using lxd, meaning updates are harder to verify. It's obviously correct, you can see just from the hunk below that it was infinitely looping before now it breaks when it found the end. diff --git a/apt-private/acqprogress.cc b/apt-private/acqprogress.cc index b37934cd4..fa7edfc82 100644 --- a/apt-private/acqprogress.cc +++ b/apt-private/acqprogress.cc @@ -322,8 +322,10 @@ bool AcqTextStatus::MediaChange(std::string Media, std::string Drive) while (C != '\n' && C != '\r') { int len = read(STDIN_FILENO,&C,1); - if(C == 'c' || len <= 0) + if(C == 'c' || len <= 0) { bStatus = false; + break; + } } if(bStatus) JSON changes only execute if you have a JSON hook configured. In the archive, only snapd has a JSON hook. It's possible that if you have snapd installed, installs could fail due to invalid JSON being sent to the snapd process and then the hook failing :/ diff --git a/apt-private/private-json-hooks.cc b/apt-private/private-json-hooks.cc index 0b765a4ed..6bf70b1c6 100644 --- a/apt-private/private-json-hooks.cc +++ b/apt-private/private-json-hooks.cc @@ -11,7 +11,9 @@ #include <apt-pkg/macros.h> #include <apt-pkg/strutl.h> #include <apt-private/private-json-hooks.h> +#include <apt-private/private-output.h> +#include <iomanip> #include <ostream> #include <sstream> #include <stack> @@ -23,7 +25,7 @@ /** * @brief Simple JSON writer * - * This performs no error checking, or string escaping, be careful. + * This performs no error checking, so be careful. */ class APT_HIDDEN JsonWriter { @@ -78,6 +80,7 @@ class APT_HIDDEN JsonWriter void popState() { this->state = old_states.top(); + old_states.pop(); } public: @@ -109,22 +112,40 @@ class APT_HIDDEN JsonWriter os << '}'; return *this; } + std::ostream &encodeString(std::ostream &out, std::string const &str) + { + out << '"'; + + for (std::string::const_iterator c = str.begin(); c != str.end(); c++) + { + if (*c <= 0x1F || *c == '"' || *c == '\\') + ioprintf(out, "\\u%04X", *c); + else + out << *c; + } + + out << '"'; + return out; + } JsonWriter &name(std::string const &name) { maybeComma(); - os << '"' << name << '"' << ':'; + encodeString(os, name) << ':'; return *this; } JsonWriter &value(std::string const &value) { maybeComma(); - os << '"' << value << '"'; + encodeString(os, value); return *this; } JsonWriter &value(const char *value) { maybeComma(); - os << '"' << value << '"'; + if (value == nullptr) + os << "null"; + else + encodeString(os, value); return *this; } JsonWriter &value(int value) Exception: This hunk is just for ordering output. Without it, the test suite would be unreliable because output from the hooks would not appear in a consistent order relative to the output from apt: @@ -315,6 +336,13 @@ bool RunJsonHook(std::string const &option, std::string const &method, const cha return true; Opts = Opts->Child; + // Flush output before calling hooks + std::clog.flush(); + std::cerr.flush(); + std::cout.flush(); + c2out.flush(); + c1out.flush(); + sighandler_t old_sigpipe = signal(SIGPIPE, SIG_IGN); sighandler_t old_sigint = signal(SIGINT, SIG_IGN); sighandler_t old_sigquit = signal(SIGQUIT, SIG_IGN); Finally, the change to make TLS errors transient is just replacing the word FATAL with TRANSIENT. It enables retries and means that switching from http:// to https:// sources behaves more consistently. It also means you can enable Acquire::Retries and have it work sensibly for servers which do accept your TCP but then fail your TLS, which happens surprisingly often. diff --git a/methods/connect.cc b/methods/connect.cc index d513a4540..044984403 100644 --- a/methods/connect.cc +++ b/methods/connect.cc @@ -1045,7 +1045,7 @@ ResultState UnwrapTLS(std::string const &Host, std::unique_ptr<MethodFd> &Fd, err = tlsFd->DoTLSHandshake(); if (err < 0) - return ResultState::FATAL_ERROR; + return ResultState::TRANSIENT_ERROR; return ResultState::SUCCESSFUL; } -- debian developer - deb.li/jak | jak-linux.org - free software dev ubuntu core developer i speak de, en
apt_2.2.4.diff.gz
Description: application/gzip
signature.asc
Description: PGP signature