Your message dated Fri, 16 Jan 2015 22:06:00 +0100 with message-id <54B97D38.7020402@thykier.net> and subject line Re: Bug#775039: pre-approval: apt/1.0.9.6 has caused the Debian Bug report #775039, regarding pre-approval: apt/1.0.9.6 to be marked as done. This means that you claim that the problem has been dealt with. If this is not the case it is now your responsibility to reopen the Bug report if necessary, and/or fix the problem forthwith. (NB: If you are a system administrator and have no idea what this message is talking about, this may indicate a serious mail system misconfiguration somewhere. Please contact owner@bugs.debian.org immediately.) -- 775039: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775039 Debian Bug Tracking System Contact owner@bugs.debian.org with problems
--- Begin Message ---
- To: Debian Bug Tracking System <submit@bugs.debian.org>
- Subject: pre-approval: apt/1.0.9.6
- From: David Kalnischkies <david@kalnischkies.de>
- Date: Sat, 10 Jan 2015 16:16:29 +0100
- Message-id: <[🔎] 20150110151629.GC30702@crossbow>
Package: release.debian.org Severity: normal User: release.debian.org@packages.debian.org Usertags: unblock X-Debbugs-CC: deity@lists.debian.org Hi release team, another year, another apt pre-approval request. In this episode: * Fix missing URIStart() for https downloads Minor annoyance at first glance, but frontends might need it for proper display. No progress report has the tendency to cause users to believe that the program crashed (or similar such) and regression potential is non-existent. * 128 KiB DSC files ought to be enough for everyone (Closes: 774893) I am a strong believer that tools in jessie should be able to deal with what is in jessie. Beside this 'obvious' fix it also fixes an overflow issue if the dsc file is nearly hitting the limit (the overflow being at most two newlines, so hardly a security problem, but a crash). Reading dsc files is the bread and butter of apt-ftparchive, so if there would be a regression it would be immediately visible. The alternative of just raising the limit (+ some additional buffer) feels incredibly dirty… This isn't effecting Debian per-se as dak and co do not use apt-ftparchive (anymore), but other distros based on jessie do use it and potentially do it for ages to come to develop the next release. * award points for positive dependencies again (Closes: 774924) I might be biased, potentially even twice, but this fixes a major mistake which potentially causes a lot of trouble (as shown in the jenkins runs) even through it managed to stay below the radar for months. The regression potential is again non-existent, mostly because this fixes a regression and back then this behaviour worked, it worked for ages. Attached as usual 'git log -p' of the involved commits. Best regards David Kalnischkiescommit 77b6f202e1629b7794a03b6522d636ff1436d074 Author: David Kalnischkies <david@kalnischkies.de> Date: Sat Jan 10 12:31:18 2015 +0100 award points for positive dependencies again Commit 9ec748ff103840c4c65471ca00d3b72984131ce4 from Feb 23 last year adds a version check after 8daf68e366fa9fa2794ae667f51562663856237c added 8 days earlier negative points for breaks/conflicts with the intended that only dependencies which are satisfied propagate points (aka: old conflicts do not). The implementation was needlessly complex and flawed through preventing positive dependencies from gaining points like they did before these commits making library transitions harder instead of simpler. It worked out anyhow most of the time out of pure 'luck' (and other ways of gaining points) or got miss attributed to being a temporary hick-up. Closes: 774924 diff --git a/apt-pkg/algorithms.cc b/apt-pkg/algorithms.cc index 608ec7f..b838310 100644 --- a/apt-pkg/algorithms.cc +++ b/apt-pkg/algorithms.cc @@ -468,7 +468,7 @@ void pkgProblemResolver::MakeScores() if (D->Version != 0) { pkgCache::VerIterator const IV = Cache[T].InstVerIter(Cache); - if (IV.end() == true || D.IsSatisfied(IV) != D.IsNegative()) + if (IV.end() == true || D.IsSatisfied(IV) == false) continue; } Scores[T->ID] += DepMap[D->Type]; diff --git a/test/integration/test-allow-scores-for-all-dependency-types b/test/integration/test-allow-scores-for-all-dependency-types index a5c98f3..d60cb8d 100755 --- a/test/integration/test-allow-scores-for-all-dependency-types +++ b/test/integration/test-allow-scores-for-all-dependency-types @@ -32,6 +32,11 @@ insertpackage 'multipleyes' 'foo' 'amd64' '2.2' 'Conflicts: bar (<= 3)' # having foo multiple times as conflict is a non-advisable hack in general insertpackage 'multipleyes' 'bar' 'amd64' '2.2' 'Conflicts: foo (<= 3), foo (<= 3)' +#774924 - slightly simplified +insertpackage 'jessie' 'login' 'amd64' '2' 'Pre-Depends: libaudit1 (>= 0)' +insertpackage 'jessie' 'libaudit1' 'amd64' '2' 'Depends: libaudit-common (>= 0)' +insertpackage 'jessie' 'libaudit-common' 'amd64' '2' 'Breaks: libaudit0, libaudit1 (<< 2)' + cp rootdir/var/lib/dpkg/status rootdir/var/lib/dpkg/status-backup setupaptarchive @@ -142,3 +147,26 @@ Inst foo [1] (2 versioned [amd64]) Inst baz (2 versioned [amd64]) Conf foo (2 versioned [amd64]) Conf baz (2 versioned [amd64])' aptget install baz -st versioned + +# recreating the exact situation is hard, so we pull tricks to get the score +cp -f rootdir/var/lib/dpkg/status-backup rootdir/var/lib/dpkg/status +insertinstalledpackage 'gdm3' 'amd64' '1' 'Depends: libaudit0, libaudit0' +insertinstalledpackage 'login' 'amd64' '1' 'Essential: yes' +insertinstalledpackage 'libaudit0' 'amd64' '1' +testequal 'Reading package lists... +Building dependency tree... +The following packages will be REMOVED: + gdm3 libaudit0 +The following NEW packages will be installed: + libaudit-common libaudit1 +The following packages will be upgraded: + login +1 upgraded, 2 newly installed, 2 to remove and 0 not upgraded. +Remv gdm3 [1] +Remv libaudit0 [1] +Inst libaudit-common (2 jessie [amd64]) +Conf libaudit-common (2 jessie [amd64]) +Inst libaudit1 (2 jessie [amd64]) +Conf libaudit1 (2 jessie [amd64]) +Inst login [1] (2 jessie [amd64]) +Conf login (2 jessie [amd64])' aptget dist-upgrade -st jessie commit 31be38d205406d4c756684e20b93d62c4701e091 Author: David Kalnischkies <david@kalnischkies.de> Date: Fri Jan 9 01:03:31 2015 +0100 128 KiB DSC files ought to be enough for everyone Your mileage may vary, but don't worry: There is more than one way to do it, but our one size fits all is not a bigger hammer, but an entire roundhouse kick! So brace yourself for the tl;dr: The limit is gone.* Beware: This fixes also the problem that a double newline is unconditionally added 'later' which is an overcommitment in case the dsc filesize is limit-2 <= x <= limit. * limited to numbers fitting into an unsigned long long. Closes: 774893 diff --git a/ftparchive/cachedb.cc b/ftparchive/cachedb.cc index 0901492..c73a64f 100644 --- a/ftparchive/cachedb.cc +++ b/ftparchive/cachedb.cc @@ -328,12 +328,12 @@ bool CacheDB::LoadSource() if (Dsc.Read(FileName) == false) return false; - if (Dsc.Data == 0) + if (Dsc.Length == 0) return _error->Error(_("Failed to read .dsc")); - + // Write back the control information InitQuerySource(); - if (Put(Dsc.Data, Dsc.Length) == true) + if (Put(Dsc.Data.c_str(), Dsc.Length) == true) CurStat.Flags |= FlSource; return true; diff --git a/ftparchive/sources.cc b/ftparchive/sources.cc index d0878a7..ab976b4 100644 --- a/ftparchive/sources.cc +++ b/ftparchive/sources.cc @@ -1,5 +1,5 @@ #include <string> -#include <iostream> +#include <sstream> // for memcpy #include <cstring> @@ -9,17 +9,19 @@ #include "sources.h" -bool DscExtract::TakeDsc(const void *newData, unsigned long newSize) +bool DscExtract::TakeDsc(const void *newData, unsigned long long newSize) { - if(newSize > maxSize) - return _error->Error("DSC data is too large %lu!", newSize); - if (newSize == 0) { + // adding two newlines 'off record' for pkgTagSection.Scan() calls + Data = "\n\n"; Length = 0; return true; } - memcpy(Data, newData, newSize); + + Data = std::string((const char*)newData, newSize); + // adding two newlines 'off record' for pkgTagSection.Scan() calls + Data.append("\n\n"); Length = newSize; return true; @@ -27,20 +29,31 @@ bool DscExtract::TakeDsc(const void *newData, unsigned long newSize) bool DscExtract::Read(std::string FileName) { + Data.clear(); + Length = 0; + FileFd F; if (OpenMaybeClearSignedFile(FileName, F) == false) return false; - - unsigned long long const FSize = F.FileSize(); - if(FSize > maxSize) - return _error->Error("DSC file '%s' is too large!",FileName.c_str()); - - if (F.Read(Data, FSize) == false) - return false; - Length = FSize; IsClearSigned = (FileName != F.Name()); + std::ostringstream data; + char buffer[1024]; + do { + unsigned long long actual = 0; + if (F.Read(buffer, sizeof(buffer)-1, &actual) == false) + return _error->Errno("read", "Failed to read dsc file %s", FileName.c_str()); + if (actual == 0) + break; + Length += actual; + buffer[actual] = '\0'; + data << buffer; + } while(true); + + // adding two newlines 'off record' for pkgTagSection.Scan() calls + data << "\n\n"; + Data = data.str(); return true; } diff --git a/ftparchive/sources.h b/ftparchive/sources.h index 91e0b13..a125ec6 100644 --- a/ftparchive/sources.h +++ b/ftparchive/sources.h @@ -3,29 +3,21 @@ #include <apt-pkg/tagfile.h> -class DscExtract +#include <string> + +class DscExtract { public: - //FIXME: do we really need to enforce a maximum size of the dsc file? - static const int maxSize = 128*1024; - - char *Data; + std::string Data; pkgTagSection Section; - unsigned long Length; + unsigned long long Length; bool IsClearSigned; - bool TakeDsc(const void *Data, unsigned long Size); + bool TakeDsc(const void *Data, unsigned long long Size); bool Read(std::string FileName); - - DscExtract() : Data(0), Length(0) { - Data = new char[maxSize]; - }; - ~DscExtract() { - if(Data != NULL) { - delete [] Data; - Data = NULL; - } - }; + + DscExtract() : Length(0), IsClearSigned(false) {}; + ~DscExtract() {}; }; diff --git a/ftparchive/writer.cc b/ftparchive/writer.cc index 7c1c9cc..0f6cc17 100644 --- a/ftparchive/writer.cc +++ b/ftparchive/writer.cc @@ -634,18 +634,10 @@ bool SourcesWriter::DoPackage(string FileName) // the "db cursor" Db.Finish(); - // read stuff - char *Start = Db.Dsc.Data; - char *BlkEnd = Db.Dsc.Data + Db.Dsc.Length; - - // Add extra \n to the end, just in case (as in clearsigned they are missing) - *BlkEnd++ = '\n'; - *BlkEnd++ = '\n'; - pkgTagSection Tags; - if (Tags.Scan(Start,BlkEnd - Start) == false) + if (Tags.Scan(Db.Dsc.Data.c_str(), Db.Dsc.Data.length()) == false) return _error->Error("Could not find a record in the DSC '%s'",FileName.c_str()); - + if (Tags.Exists("Source") == false) return _error->Error("Could not find a Source entry in the DSC '%s'",FileName.c_str()); Tags.Trim(); diff --git a/test/integration/framework b/test/integration/framework index c944506..70ad381 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -780,7 +780,7 @@ buildaptarchivefromincoming() { [ -e ftparchive.conf ] || createaptftparchiveconfig [ -e dists ] || buildaptftparchivedirectorystructure msgninfo "\tGenerate Packages, Sources and Contents files… " - aptftparchive -qq generate ftparchive.conf + testsuccess aptftparchive generate ftparchive.conf cd - > /dev/null msgdone "info" generatereleasefiles diff --git a/test/integration/test-apt-ftparchive-src-cachedb b/test/integration/test-apt-ftparchive-src-cachedb index adcca62..0ac4d55 100755 --- a/test/integration/test-apt-ftparchive-src-cachedb +++ b/test/integration/test-apt-ftparchive-src-cachedb @@ -180,10 +180,6 @@ testequal " E: Could not find a Source entry in the DSC 'aptarchive/pool/invalid/invalid_1.0.dsc'" aptftparchive sources aptarchive/pool/invalid rm -f aptarchive/pool/invalid/invalid_1.0.dsc -dd if=/dev/zero of="aptarchive/pool/invalid/toobig_1.0.dsc" bs=1k count=129 2>/dev/null -testequal " -E: DSC file 'aptarchive/pool/invalid/toobig_1.0.dsc' is too large!" aptftparchive sources aptarchive/pool/invalid - # ensure clean works rm -f aptarchive/pool/main/* aptftparchive clean apt-ftparchive.conf -o Debug::APT::FTPArchive::Clean=1 > clean-out.txt 2>&1 commit d13f2ef5dd2cf41d7abd7f309a9e8965a77d2a63 Author: Michael Vogt <mvo@ubuntu.com> Date: Tue Jan 6 10:54:24 2015 +0100 Add regression test for the previous commit The issue was that https.cc never called URIStart(), one way to detect this is that no download progress is generated without this call. The test now checks for this and as a side-effect will also ensure that we do not break download progress reporting and Acquire::{http,https}::Dl-Limit accidently. diff --git a/test/integration/test-apt-download-progress b/test/integration/test-apt-download-progress new file mode 100755 index 0000000..0a9020b --- /dev/null +++ b/test/integration/test-apt-download-progress @@ -0,0 +1,43 @@ +#!/bin/sh +# +# ensure downloading sends progress as a regression test for commit 9127d7ae +# +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +changetohttpswebserver + +assertprogress() { + T="$1" + testsuccess grep "dlstatus:1:0:Retrieving file 1 of 1" "$T" + if ! egrep -q "dlstatus:1:[0-9]{1,2}\.(.*):Retrieving file 1 of 1" "$T"; then + cat "$T" + msgfail "Failed to detect download progress" + fi + testsuccess grep "dlstatus:1:100:Retrieving file 1 of 1" "$T" + #cat $T +} + +# we need to ensure the file is reasonable big so that apt has a chance to +# actually report progress - but not too big to ensure its not delaying the +# test too much +TESTFILE=testfile.big +testsuccess dd if=/dev/zero of=./aptarchive/$TESTFILE bs=800k count=1 + +msgtest 'download progress works via' 'http' +printf '\n' +exec 3> apt-progress.log +testsuccess apthelper download-file "http://localhost:8080/$TESTFILE"; http-$TESTFILE -o APT::Status-Fd=3 -o Acquire::http::Dl-Limit=800 +assertprogress apt-progress.log + +msgtest 'download progress works via' 'https' +printf '\n' +exec 3> apt-progress.log +testsuccess apthelper download-file "https://localhost:4433/$TESTFILE"; https-$TESTFILE -o APT::Status-Fd=3 -o Acquire::https::Dl-Limit=800 +assertprogress apt-progress.log + +# cleanup +rm -f apt-progress*.log commit 9127d7aecf01f2999a2589e4b0503288518b2927 Author: Michael Vogt <mvo@ubuntu.com> Date: Mon Jan 5 10:27:53 2015 +0100 Fix missing URIStart() for https downloads Add a explicit ReceivedData to HttpsMethod that indicates when we got data from the connection so that we can send URISTart() to the parent. This is needed because URIStart got moved in f9b4f12d from the progress_callback to write_data() and it only checks for Res.Size. In the old code if progress_callback is called by libcurl (and sets Res.Size) before write_data is called then URIStart() is never send. Making this a explicit ReceivedData variable fixes this issue. diff --git a/methods/https.cc b/methods/https.cc index 65a744e..3a5981b 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -85,8 +85,12 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) if (me->Server->JunkSize != 0) return buffer_size; - if (me->Res.Size == 0) + if (me->ReceivedData == false) + { me->URIStart(me->Res); + me->ReceivedData = true; + } + if(me->File->Write(buffer, buffer_size) != true) return false; @@ -95,7 +99,7 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) int HttpsMethod::progress_callback(void *clientp, double dltotal, double /*dlnow*/, - double /*ultotal*/, double /*ulnow*/) + double /*ultotal*/, double /*ulnow*/) { HttpsMethod *me = (HttpsMethod *)clientp; if(dltotal > 0 && me->Res.Size == 0) { @@ -179,6 +183,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) char curl_errorstr[CURL_ERROR_SIZE]; URI Uri = Itm->Uri; string remotehost = Uri.Host; + ReceivedData = false; // TODO: // - http::Pipeline-Depth diff --git a/methods/https.h b/methods/https.h index faac8a3..411b714 100644 --- a/methods/https.h +++ b/methods/https.h @@ -66,6 +66,7 @@ class HttpsMethod : public pkgAcqMethod CURL *curl; FetchResult Res; HttpsServerState *Server; + bool ReceivedData; public: FileFd *File;Attachment: signature.asc
Description: Digital signature
--- End Message ---
--- Begin Message ---
- To: David Kalnischkies <david@kalnischkies.de>, 775039-done@bugs.debian.org
- Subject: Re: Bug#775039: pre-approval: apt/1.0.9.6
- From: Niels Thykier <niels@thykier.net>
- Date: Fri, 16 Jan 2015 22:06:00 +0100
- Message-id: <54B97D38.7020402@thykier.net>
- In-reply-to: <[🔎] 54B54549.1090304@thykier.net>
- References: <[🔎] 20150110151629.GC30702@crossbow> <[🔎] 54B54549.1090304@thykier.net>
On 2015-01-13 17:18, Niels Thykier wrote: > Control: tags -1 moreinfo confirmed > > [..] > > Hi > > Ack, please go ahead with the apt/1.0.9.6 upload. > > ~Niels > > > This was uploaded, and I have unblocked it and aged it to 2 days. Thanks, ~Niels
--- End Message ---