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
