From b4b5e9bf97970e0efc4a994de96066a92e3a9b8f Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 2 Dec 2020 15:46:34 +0100 Subject: Use 500 MB memory limit for xz/lzma decoding The buffers we feed in and read out are usually a couple kilobytes big so allowing lzma to use an unlimited amount of memory is easy & okay, but not needed and confuses memory checkers as it will cause lzma to malloc a huge chunk of memory (which it will never use). So lets just use a "big enough" value instead. In exchange we simplify the decoder calling as we were already using the auto-variant for xz, so we can just avoid the if-else and let liblzma decide what it decodes. --- apt-pkg/contrib/fileutl.cc | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index e91c1acc3..091def3d4 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -1774,9 +1774,8 @@ public: #endif }; /*}}}*/ - -class APT_HIDDEN ZstdFileFdPrivate : public FileFdPrivate -{ /*{{{*/ +class APT_HIDDEN ZstdFileFdPrivate : public FileFdPrivate /*{{{*/ +{ #ifdef HAVE_ZSTD ZSTD_DStream *dctx; ZSTD_CStream *cctx; @@ -1986,7 +1985,7 @@ class APT_HIDDEN ZstdFileFdPrivate : public FileFdPrivate #endif }; /*}}}*/ -class APT_HIDDEN LzmaFileFdPrivate: public FileFdPrivate { /*{{{*/ +class APT_HIDDEN LzmaFileFdPrivate: public FileFdPrivate { /*{{{*/ #ifdef HAVE_LZMA struct LZMAFILE { FILE* file; @@ -2092,17 +2091,9 @@ public: } else { - uint64_t const memlimit = UINT64_MAX; - if (compressor.Name == "xz") - { - if (lzma_auto_decoder(&lzma->stream, memlimit, 0) != LZMA_OK) - return false; - } - else - { - if (lzma_alone_decoder(&lzma->stream, memlimit) != LZMA_OK) - return false; - } + uint64_t constexpr memlimit = 1024 * 1024 * 500; + if (lzma_auto_decoder(&lzma->stream, memlimit, 0) != LZMA_OK) + return false; lzma->compressing = false; } return true; -- cgit v1.2.3-70-g09d2 From f2c087449286812823d06d1b560fa947e438fa0d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 2 Dec 2020 15:51:09 +0100 Subject: Guess compressor only if no AR nember with exact name exists Explicitly opening a tar member is a bit harder than it needs to be as you have to remove the compressor extension so that it can be guessed here gain potentially choosing the wrong member. Doesn't really matter for deb packages of course as the member count is pretty low and strongly defined, but testing is easier this way. It also finally fixes an incorrectly formatted error message. --- apt-pkg/deb/debfile.cc | 59 ++++++++++++++++++------------- test/integration/test-apt-get-install-deb | 2 +- 2 files changed, 35 insertions(+), 26 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/deb/debfile.cc b/apt-pkg/deb/debfile.cc index bef0cd0d8..645a579ef 100644 --- a/apt-pkg/deb/debfile.cc +++ b/apt-pkg/deb/debfile.cc @@ -24,9 +24,12 @@ #include #include #include +#include #include +#include #include +#include #include #include #include @@ -105,42 +108,48 @@ const ARArchive::Member *debDebFile::GotoMember(const char *Name) /* Simple wrapper around tar.. */ bool debDebFile::ExtractTarMember(pkgDirStream &Stream,const char *Name) { - // Get the archive member - const ARArchive::Member *Member = NULL; std::string Compressor; + auto const Compressors = APT::Configuration::getCompressors(); - std::vector compressor = APT::Configuration::getCompressors(); - for (std::vector::const_iterator c = compressor.begin(); - c != compressor.end(); ++c) + ARArchive::Member const *Member = AR.FindMember(Name); + if (Member != nullptr) { - Member = AR.FindMember(std::string(Name).append(c->Extension).c_str()); - if (Member == NULL) - continue; - Compressor = c->Name; - break; + auto const found = std::find_if(Compressors.cbegin(), Compressors.cend(), [&](auto const &c) { + return not c.Extension.empty() && APT::String::Endswith(Name, c.Extension); + }); + if (found != Compressors.cend()) + Compressor = found->Name; } - - if (Member == NULL) - Member = AR.FindMember(std::string(Name).c_str()); - - if (Member == NULL) + else { - std::string ext = std::string(Name) + ".{"; - for (std::vector::const_iterator c = compressor.begin(); - c != compressor.end(); ++c) { - if (!c->Extension.empty()) - ext.append(c->Extension.substr(1)); + for (auto const &c : Compressors) + { + if (c.Extension.empty()) + continue; + Member = AR.FindMember(std::string(Name).append(c.Extension).c_str()); + if (Member == nullptr) + continue; + Compressor = c.Name; + break; } - ext.append("}"); - return _error->Error(_("Internal error, could not locate member %s"), ext.c_str()); } - if (File.Seek(Member->Start) == false) + if (Member == nullptr) + { + std::ostringstream ext; + ext << Name << '{'; + for (auto const &c : Compressors) + if (not c.Extension.empty()) + ext << c.Extension << ','; + ext << '}'; + return _error->Error(_("Internal error, could not locate member %s"), ext.str().c_str()); + } + + if (not File.Seek(Member->Start)) return false; - // Prepare Tar ExtractTar Tar(File,Member->Size,Compressor); - if (_error->PendingError() == true) + if (_error->PendingError()) return false; return Tar.Go(Stream); } diff --git a/test/integration/test-apt-get-install-deb b/test/integration/test-apt-get-install-deb index 7fa5ca3e7..6fde00708 100755 --- a/test/integration/test-apt-get-install-deb +++ b/test/integration/test-apt-get-install-deb @@ -23,7 +23,7 @@ mv foo.rpm foo.deb for exe in apt aptget; do for cmd in install remove purge upgrade full-upgrade; do testfailuremsg "E: Invalid archive signature -E: Internal error, could not locate member control.tar.{zstlz4gzxzbz2lzma} +E: Internal error, could not locate member control.tar{.zst,.lz4,.gz,.xz,.bz2,.lzma,} E: Could not read meta data from ${TMPWORKINGDIRECTORY}/foo.deb E: The package lists or status file could not be parsed or opened." $exe $cmd ./foo.deb done -- cgit v1.2.3-70-g09d2 From cd1f75854824b3db7ba86039463528f834a0838a Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 10:55:09 +0100 Subject: Free XXH3 state to avoid leak in cache hashing We do this once (usually), so the leak is tremendously big, but it is detected as a leak by the fuzzer and trips it up. --- apt-pkg/pkgcache.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/pkgcache.cc b/apt-pkg/pkgcache.cc index f7f3537aa..cbde7c42f 100644 --- a/apt-pkg/pkgcache.cc +++ b/apt-pkg/pkgcache.cc @@ -255,7 +255,9 @@ uint32_t pkgCache::CacheHash() GetMap().Size() - sizeof(header)); } - return XXH3_64bits_digest(state) & 0xFFFFFFFF; + auto const digest = XXH3_64bits_digest(state); + XXH3_freeState(state); + return digest & 0xFFFFFFFF; } /*}}}*/ // Cache::FindPkg - Locate a package by name /*{{{*/ -- cgit v1.2.3-70-g09d2 From bb553c3884f811792a1d8021ea8396cbe1ec0b23 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:19:21 +0100 Subject: Fail ConfigDir reading if directory listing failed We were printing an error and hence have non-zero exit code either way, but API wise it makes sense to have this properly reported back to the caller to propagate it down the chain e.g. while parsing #include stanzas. --- apt-pkg/contrib/configuration.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/configuration.cc b/apt-pkg/contrib/configuration.cc index 931df9f6c..0c98ff304 100644 --- a/apt-pkg/contrib/configuration.cc +++ b/apt-pkg/contrib/configuration.cc @@ -1150,10 +1150,13 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio bool ReadConfigDir(Configuration &Conf,const string &Dir, bool const &AsSectional, unsigned const &Depth) { + _error->PushToStack(); auto const files = GetListOfFilesInDir(Dir, "conf", true, true); + auto const successfulList = not _error->PendingError(); + _error->MergeWithStack(); return std::accumulate(files.cbegin(), files.cend(), true, [&](bool good, auto const &file) { return ReadConfigFile(Conf, file, AsSectional, Depth) && good; - }); + }) && successfulList; } /*}}}*/ // MatchAgainstConfig Constructor /*{{{*/ -- cgit v1.2.3-70-g09d2 From 03790b071d6a97374a03af36eda5698a143300b0 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:29:54 +0100 Subject: Retire and deprecate _strtabexpand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the Configuration code calling this was any indication, it is hard to use – and even that monster still caused heap-buffer-overflow errors, so instead of trying to fix it, lets just use methods which are far easier to use. The question why this is done at all remains, but is left for another day as an exercise for the reader. --- apt-pkg/cdrom.cc | 31 ++++++++++++------------------- apt-pkg/contrib/configuration.cc | 22 ++-------------------- apt-pkg/contrib/strutl.h | 2 +- 3 files changed, 15 insertions(+), 40 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/cdrom.cc b/apt-pkg/cdrom.cc index 5df03a99c..aacb78bba 100644 --- a/apt-pkg/cdrom.cc +++ b/apt-pkg/cdrom.cc @@ -464,14 +464,12 @@ bool pkgCdrom::WriteSourceList(string Name,vector &List,bool Source) string File = _config->FindFile("Dir::Etc::sourcelist"); - // Open the stream for reading - ifstream F((FileExists(File)?File.c_str():"/dev/null"), - ios::in ); - if (F.fail() == true) - return _error->Errno("ifstream::ifstream","Opening %s",File.c_str()); + FileFd F(FileExists(File) ? File : "/dev/null", FileFd::ReadOnly); + if (not F.IsOpen() || F.Failed()) + return _error->Errno("WriteSourceList", "Opening %s failed", File.c_str()); string NewFile = File + ".new"; - RemoveFile("WriteDatabase", NewFile); + RemoveFile("WriteSourceList", NewFile); ofstream Out(NewFile.c_str()); if (!Out) return _error->Errno("ofstream::ofstream", @@ -487,21 +485,16 @@ bool pkgCdrom::WriteSourceList(string Name,vector &List,bool Source) else Type = "deb"; - char Buffer[300]; int CurLine = 0; bool First = true; - while (F.eof() == false) - { - F.getline(Buffer,sizeof(Buffer)); - CurLine++; - if (F.fail() && !F.eof()) - return _error->Error(_("Line %u too long in source list %s."), - CurLine,File.c_str()); - _strtabexpand(Buffer,sizeof(Buffer)); - _strstrip(Buffer); - + std::string Buffer; + while (F.ReadLine(Buffer)) + { + ++CurLine; + auto const Cleaned = APT::String::Strip(SubstVar(Buffer, "\t", " ")); + // Comment or blank - if (Buffer[0] == '#' || Buffer[0] == 0) + if (Cleaned.empty() || Cleaned[0] == '#') { Out << Buffer << endl; continue; @@ -523,7 +516,7 @@ bool pkgCdrom::WriteSourceList(string Name,vector &List,bool Source) // Grok it string cType; string URI; - const char *C = Buffer; + const char *C = Cleaned.c_str(); if (ParseQuoteWord(C,cType) == false || ParseQuoteWord(C,URI) == false) { diff --git a/apt-pkg/contrib/configuration.cc b/apt-pkg/contrib/configuration.cc index 0c98ff304..554307324 100644 --- a/apt-pkg/contrib/configuration.cc +++ b/apt-pkg/contrib/configuration.cc @@ -861,26 +861,8 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio // The input line with comments stripped. std::string Fragment; - // Expand tabs in the input line and remove leading and trailing - // whitespace. - { - const int BufferSize = Input.size() * 8 + 1; - char *Buffer = new char[BufferSize]; - try - { - memcpy(Buffer, Input.c_str(), Input.size() + 1); - - _strtabexpand(Buffer, BufferSize); - _strstrip(Buffer); - Input = Buffer; - } - catch(...) - { - delete[] Buffer; - throw; - } - delete[] Buffer; - } + // Expand tabs in the input line and remove leading and trailing whitespace. + Input = APT::String::Strip(SubstVar(Input, "\t", " ")); CurLine++; // Now strip comments; if the whole line is contained in a diff --git a/apt-pkg/contrib/strutl.h b/apt-pkg/contrib/strutl.h index c25c6208a..8669c97a5 100644 --- a/apt-pkg/contrib/strutl.h +++ b/apt-pkg/contrib/strutl.h @@ -42,7 +42,7 @@ namespace APT { APT_PUBLIC bool UTF8ToCodeset(const char *codeset, const std::string &orig, std::string *dest); APT_PUBLIC char *_strstrip(char *String); APT_PUBLIC char *_strrstrip(char *String); // right strip only -APT_PUBLIC char *_strtabexpand(char *String,size_t Len); +APT_DEPRECATED_MSG("Use SubstVar to avoid memory headaches") APT_PUBLIC char *_strtabexpand(char *String,size_t Len); APT_PUBLIC bool ParseQuoteWord(const char *&String,std::string &Res); APT_PUBLIC bool ParseCWord(const char *&String,std::string &Res); APT_PUBLIC std::string QuoteString(const std::string &Str,const char *Bad); -- cgit v1.2.3-70-g09d2 From 10f13938bbf1474451fadcd62e1c31c4b5f5b3d7 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:41:43 +0100 Subject: Fix incorrect base64 encoding due to int promotion For \xff and friends with the highest bit set and hence being a negative value on signed char systems the wrong encoding is produced as we run into undefined behaviour accessing negative array indexes. We can avoid this problem simply by using an unsigned data type. --- apt-pkg/contrib/strutl.cc | 2 +- test/libapt/strutil_test.cc | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index bd4856526..826b21478 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -597,7 +597,7 @@ string Base64Encode(const string &S) base64. */ for (string::const_iterator I = S.begin(); I < S.end(); I += 3) { - char Bits[3] = {0,0,0}; + uint8_t Bits[3] = {0,0,0}; Bits[0] = I[0]; if (I + 1 < S.end()) Bits[1] = I[1]; diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index 349946b5f..f101d72cf 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -167,6 +167,10 @@ TEST(StrUtilTest,Base64Encode) EXPECT_EQ("ZS4=", Base64Encode("e.")); EXPECT_EQ("Lg==", Base64Encode(".")); EXPECT_EQ("", Base64Encode("")); + EXPECT_EQ("IA==", Base64Encode("\x20")); + EXPECT_EQ("/w==", Base64Encode("\xff")); + EXPECT_EQ("/A==", Base64Encode("\xfc")); + EXPECT_EQ("//8=", Base64Encode("\xff\xff")); } static void ReadMessagesTestWithNewLine(char const * const nl, char const * const ab) { -- cgit v1.2.3-70-g09d2 From ed192f410da36aedf5e54bb3f967e6613ab4bb51 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:44:27 +0100 Subject: Don't parse \x and \0 past the end in DeEscapeString MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This has no attack surface though as the loop is to end very soon anyhow and the method only used while reading CD-ROM mountpoints which seems like a very unlikely attack vector… --- apt-pkg/contrib/strutl.cc | 12 ++++++++---- test/libapt/strutil_test.cc | 6 ++++++ 2 files changed, 14 insertions(+), 4 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 826b21478..45e475b3e 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -1611,22 +1611,26 @@ string DeEscapeString(const string &input) switch (*it) { case '0': - if (it + 2 <= input.end()) { + if (it + 2 < input.end()) { tmp[0] = it[1]; tmp[1] = it[2]; tmp[2] = 0; output += (char)strtol(tmp, 0, 8); it += 2; - } + } else { + // FIXME: raise exception here? + } break; case 'x': - if (it + 2 <= input.end()) { + if (it + 2 < input.end()) { tmp[0] = it[1]; tmp[1] = it[2]; tmp[2] = 0; output += (char)strtol(tmp, 0, 16); it += 2; - } + } else { + // FIXME: raise exception here? + } break; default: // FIXME: raise exception here? diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index f101d72cf..d477e953c 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -21,6 +21,12 @@ TEST(StrUtilTest,DeEscapeString) // double slashes EXPECT_EQ("foo\\ x", DeEscapeString("foo\\\\ x")); EXPECT_EQ("\\foo\\", DeEscapeString("\\\\foo\\\\")); + + // FIXME: the input is bad, the output as well, but we have no indicator for it + EXPECT_EQ("aa", DeEscapeString("aa\\x")); + EXPECT_EQ("aa0", DeEscapeString("aa\\x0")); + EXPECT_EQ("aa", DeEscapeString("aa\\0")); + EXPECT_EQ("aaa", DeEscapeString("aa\\0a")); } TEST(StrUtilTest,StringStrip) { -- cgit v1.2.3-70-g09d2 From 6630c3e5b6af77205b043208ef15720cf270075c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 11:12:45 +0100 Subject: Remove Word size limit from ParseQuote and CWord It isn't super likely that we will encounter such big words in the real world, but we can return arbitrary length, so lets just do that as that also means we don't have to work with a second buffer. --- apt-pkg/contrib/strutl.cc | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 45e475b3e..7beb5f621 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -310,11 +310,11 @@ bool ParseQuoteWord(const char *&String,string &Res) } // Now de-quote characters - char Buffer[1024]; + Res.clear(); + Res.reserve(C - String); char Tmp[3]; const char *Start = String; - char *I; - for (I = Buffer; I < Buffer + sizeof(Buffer) && Start != C; I++) + while (Start != C) { if (*Start == '%' && Start + 2 < C && isxdigit(Start[1]) && isxdigit(Start[2])) @@ -322,19 +322,15 @@ bool ParseQuoteWord(const char *&String,string &Res) Tmp[0] = Start[1]; Tmp[1] = Start[2]; Tmp[2] = 0; - *I = (char)strtol(Tmp,0,16); + Res.push_back(static_cast(strtol(Tmp, 0, 16))); Start += 3; continue; } if (*Start != '"') - *I = *Start; - else - I--; - Start++; + Res.push_back(*Start); + ++Start; } - *I = 0; - Res = Buffer; - + // Skip ending white space for (; isspace(*C) != 0; C++) ; @@ -354,33 +350,28 @@ bool ParseCWord(const char *&String,string &Res) ; if (*C == 0) return false; - - char Buffer[1024]; - char *Buf = Buffer; - if (strlen(String) >= sizeof(Buffer)) - return false; - - for (; *C != 0; C++) + + Res.clear(); + Res.reserve(strlen(String)); + for (; *C != 0; ++C) { if (*C == '"') { for (C++; *C != 0 && *C != '"'; C++) - *Buf++ = *C; - + Res.push_back(*C); + if (*C == 0) return false; - + continue; - } - + } + if (C != String && isspace(*C) != 0 && isspace(C[-1]) != 0) continue; if (isspace(*C) == 0) return false; - *Buf++ = ' '; + Res.push_back(' '); } - *Buf = 0; - Res = Buffer; String = C; return true; } -- cgit v1.2.3-70-g09d2 From e0743a85c5f5f2f83d91c305450e8ba192194cd8 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 13 May 2020 09:07:19 +0200 Subject: Forbid negative values in unsigned StrToNum explicitly strtoul(l) surprises us with parsing negative values which should not exist in the places we use to parse them, so we can just downright refuse them rather than trying to work with them by having them promoted to huge positive values. --- apt-pkg/contrib/strutl.cc | 41 +++++++++++++-------------------- test/libapt/strutil_test.cc | 56 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 25 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 7beb5f621..84e79122e 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -1139,34 +1140,24 @@ bool FTPMDTMStrToTime(const char* const str,time_t &time) /*}}}*/ // StrToNum - Convert a fixed length string to a number /*{{{*/ // --------------------------------------------------------------------- -/* This is used in decoding the crazy fixed length string headers in +/* This is used in decoding the crazy fixed length string headers in tar and ar files. */ bool StrToNum(const char *Str,unsigned long &Res,unsigned Len,unsigned Base) { - char S[30]; - if (Len >= sizeof(S)) + unsigned long long BigRes; + if (not StrToNum(Str, BigRes, Len, Base)) return false; - memcpy(S,Str,Len); - S[Len] = 0; - - // All spaces is a zero - Res = 0; - unsigned I; - for (I = 0; S[I] == ' '; I++); - if (S[I] == 0) - return true; - - char *End; - Res = strtoul(S,&End,Base); - if (End == S) + + if (std::numeric_limits::max() < BigRes) return false; - + + Res = BigRes; return true; } /*}}}*/ // StrToNum - Convert a fixed length string to a number /*{{{*/ // --------------------------------------------------------------------- -/* This is used in decoding the crazy fixed length string headers in +/* This is used in decoding the crazy fixed length string headers in tar and ar files. */ bool StrToNum(const char *Str,unsigned long long &Res,unsigned Len,unsigned Base) { @@ -1175,20 +1166,20 @@ bool StrToNum(const char *Str,unsigned long long &Res,unsigned Len,unsigned Base return false; memcpy(S,Str,Len); S[Len] = 0; - + // All spaces is a zero Res = 0; unsigned I; - for (I = 0; S[I] == ' '; I++); + for (I = 0; S[I] == ' '; ++I); if (S[I] == 0) return true; - + if (S[I] == '-') + return false; + char *End; + errno = 0; Res = strtoull(S,&End,Base); - if (End == S) - return false; - - return true; + return not (End == S || errno != 0); } /*}}}*/ diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index d477e953c..469de4403 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -1,6 +1,8 @@ #include #include +#include #include +#include #include #include @@ -257,6 +259,60 @@ TEST(StrUtilTest,QuoteString) EXPECT_EQ("Eltville-Erbach", DeQuoteString(QuoteString("Eltville-Erbach", ""))); } +static void EXPECT_STRTONUM(APT::StringView const str, bool const success, unsigned long const expected, unsigned const base) +{ + SCOPED_TRACE(std::string(str.data(), str.length())); + SCOPED_TRACE(base); + unsigned long N1 = 1000; + unsigned long long N2 = 1000; + if (not success) + { + EXPECT_FALSE(StrToNum(str.data(), N1, str.length(), base)); + EXPECT_FALSE(StrToNum(str.data(), N2, str.length(), base)); + return; + } + EXPECT_TRUE(StrToNum(str.data(), N1, str.length(), base)); + EXPECT_EQ(expected, N1); + + EXPECT_TRUE(StrToNum(str.data(), N2, str.length(), base)); + EXPECT_EQ(expected, N2); +} +TEST(StrUtilTest,StrToNum) +{ + EXPECT_STRTONUM("", true, 0, 10); + EXPECT_STRTONUM(" ", true, 0, 10); + EXPECT_STRTONUM("0", true, 0, 10); + EXPECT_STRTONUM("1", true, 1, 10); + EXPECT_STRTONUM(" 1 ", true, 1, 10); + EXPECT_STRTONUM("1", true, 1, 8); + EXPECT_STRTONUM("10", true, 10, 10); + EXPECT_STRTONUM("10", true, 8, 8); + EXPECT_STRTONUM("010", true, 8, 8); + EXPECT_STRTONUM(" 010 ", true, 8, 8); + EXPECT_STRTONUM("-1", false, 0, 10); + EXPECT_STRTONUM(" -1 ", false, 0, 10); + EXPECT_STRTONUM("11", true, 3, 2); + + unsigned long long bigN = 0; + unsigned long smallN = 0; + auto bigLimit = std::to_string(std::numeric_limits::max()); + if (std::numeric_limits::max() < std::numeric_limits::max()) + { + EXPECT_TRUE(StrToNum(bigLimit.c_str(), bigN, bigLimit.length(), 10)); + EXPECT_EQ(std::numeric_limits::max(), bigN); + EXPECT_FALSE(StrToNum(bigLimit.c_str(), smallN, bigLimit.length(), 10)); + } + bigLimit.append("0"); + EXPECT_FALSE(StrToNum(bigLimit.c_str(), bigN, bigLimit.length(), 10)); + EXPECT_FALSE(StrToNum(bigLimit.c_str(), smallN, bigLimit.length(), 10)); + + auto const smallLimit = std::to_string(std::numeric_limits::max()); + EXPECT_TRUE(StrToNum(smallLimit.c_str(), bigN, smallLimit.length(), 10)); + EXPECT_EQ(std::numeric_limits::max(), bigN); + EXPECT_TRUE(StrToNum(smallLimit.c_str(), smallN, smallLimit.length(), 10)); + EXPECT_EQ(std::numeric_limits::max(), smallN); +} + TEST(StrUtilTest,RFC1123StrToTime) { { -- cgit v1.2.3-70-g09d2 From c4da2ff42da55ffc38c77a9170dc151216d75962 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 17:40:05 +0100 Subject: Avoid overstepping bounds in config file parsing Our configuration files are not security relevant, but having a parser which avoids crashing on them even if they are seriously messed up is not a bad idea anyway. It is also a good opportunity to brush up the code a bit avoiding a few small string copies with our string_view. --- apt-pkg/contrib/configuration.cc | 114 +++++++++++++++++++-------------------- apt-pkg/contrib/string_view.h | 26 +++++++++ test/libapt/stringview_test.cc | 8 +++ 3 files changed, 88 insertions(+), 60 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/configuration.cc b/apt-pkg/contrib/configuration.cc index 554307324..a8fced724 100644 --- a/apt-pkg/contrib/configuration.cc +++ b/apt-pkg/contrib/configuration.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -867,83 +868,76 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio // Now strip comments; if the whole line is contained in a // comment, skip this line. + APT::StringView Line{Input.data(), Input.size()}; - // The first meaningful character in the current fragment; will - // be adjusted below as we remove bytes from the front. - std::string::const_iterator Start = Input.begin(); - // The last meaningful character in the current fragment. - std::string::const_iterator End = Input.end(); - - // Multi line comment - if (InComment == true) + // continued Multi line comment + if (InComment) { - for (std::string::const_iterator I = Start; - I != End; ++I) + size_t end = Line.find("*/"); + if (end != APT::StringView::npos) { - if (*I == '*' && I + 1 != End && I[1] == '/') - { - Start = I + 2; - InComment = false; - break; - } + Line.remove_prefix(end + 2); + InComment = false; } - if (InComment == true) + else continue; } // Discard single line comments - bool InQuote = false; - for (std::string::const_iterator I = Start; - I != End; ++I) { - if (*I == '"') - InQuote = !InQuote; - if (InQuote == true) - continue; - - if ((*I == '/' && I + 1 != End && I[1] == '/') || - (*I == '#' && strcmp(string(I,I+6).c_str(),"#clear") != 0 && - strcmp(string(I,I+8).c_str(),"#include") != 0 && - strcmp(string(I,I+strlen("#x-apt-configure-index")).c_str(), "#x-apt-configure-index") != 0)) + size_t start = 0; + while ((start = Line.find("//", start)) != APT::StringView::npos) + { + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0) + { + ++start; + continue; + } + Line.remove_suffix(Line.length() - start); + break; + } + using APT::operator""_sv; + constexpr std::array magicComments { "clear"_sv, "include"_sv, "x-apt-configure-index"_sv }; + start = 0; + while ((start = Line.find('#', start)) != APT::StringView::npos) { - End = I; + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0 || + std::any_of(magicComments.begin(), magicComments.end(), [&](auto const m) { return Line.compare(start+1, m.length(), m) == 0; })) + { + ++start; + continue; + } + Line.remove_suffix(Line.length() - start); break; } } // Look for multi line comments and build up the // fragment. - Fragment.reserve(End - Start); - InQuote = false; - for (std::string::const_iterator I = Start; - I != End; ++I) + Fragment.reserve(Line.length()); { - if (*I == '"') - InQuote = !InQuote; - if (InQuote == true) - Fragment.push_back(*I); - else if (*I == '/' && I + 1 != End && I[1] == '*') - { - InComment = true; - for (std::string::const_iterator J = I; - J != End; ++J) + size_t start = 0; + while ((start = Line.find("/*", start)) != APT::StringView::npos) + { + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0) { - if (*J == '*' && J + 1 != End && J[1] == '/') - { - // Pretend we just finished walking over the - // comment, and don't add anything to the output - // fragment. - I = J + 1; - InComment = false; - break; - } + start += 2; + continue; } - - if (InComment == true) - break; + Fragment.append(Line.data(), start); + auto const end = Line.find("*/", start + 2); + if (end == APT::StringView::npos) + { + Line.clear(); + InComment = true; + break; + } + else + Line.remove_prefix(end + 2); + start = 0; } - else - Fragment.push_back(*I); + if (not Line.empty()) + Fragment.append(Line.data(), Line.length()); } // Skip blank lines. @@ -951,9 +945,9 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio continue; // The line has actual content; interpret what it means. - InQuote = false; - Start = Fragment.begin(); - End = Fragment.end(); + bool InQuote = false; + auto Start = Fragment.cbegin(); + auto End = Fragment.cend(); for (std::string::const_iterator I = Start; I != End; ++I) { diff --git a/apt-pkg/contrib/string_view.h b/apt-pkg/contrib/string_view.h index 05aad3327..04f6ff115 100644 --- a/apt-pkg/contrib/string_view.h +++ b/apt-pkg/contrib/string_view.h @@ -39,6 +39,10 @@ public: StringView(const char *data) : data_(data), size_(strlen(data)) {} StringView(std::string const & str): data_(str.data()), size_(str.size()) {} + /* Modifiers */ + void remove_prefix(size_t n) { data_ += n; size_ -= n; } + void remove_suffix(size_t n) { size_ -= n; } + void clear() { size_ = 0; } /* Viewers */ constexpr StringView substr(size_t pos, size_t n = npos) const { @@ -76,6 +80,28 @@ public: return found - data_; } + size_t find(APT::StringView const needle) const { + if (needle.empty()) + return npos; + if (needle.length() == 1) + return find(*needle.data()); + size_t found = 0; + while ((found = find(*needle.data(), found)) != npos) { + if (compare(found, needle.length(), needle) == 0) + return found; + ++found; + } + return found; + } + size_t find(APT::StringView const needle, size_t pos) const { + if (pos == 0) + return find(needle); + size_t const found = substr(pos).find(needle); + if (found == npos) + return npos; + return pos + found; + } + /* Conversions */ std::string to_string() const { return std::string(data_, size_); diff --git a/test/libapt/stringview_test.cc b/test/libapt/stringview_test.cc index 03d82517b..9cfaa3b48 100644 --- a/test/libapt/stringview_test.cc +++ b/test/libapt/stringview_test.cc @@ -74,6 +74,14 @@ TEST(StringViewTest,Find) EXPECT_EQ(defString.to_string().find('e',3), defString.find('e',3)); EXPECT_EQ(defString.to_string().find('l',6), defString.find('l',6)); EXPECT_EQ(defString.to_string().find('l',11), defString.find('l',11)); + + EXPECT_EQ(defString.to_string().find("l"), defString.find("l")); + EXPECT_EQ(defString.to_string().find("ll"), defString.find("ll")); + EXPECT_EQ(defString.to_string().find("lo"), defString.find("lo")); + EXPECT_EQ(defString.to_string().find("ll", 1), defString.find("ll", 1)); + EXPECT_EQ(defString.to_string().find("ll", 6), defString.find("ll", 6)); + EXPECT_EQ(defString.to_string().find("or"), defString.find("or")); + EXPECT_EQ(defString.to_string().find("od"), defString.find("od")); } TEST(StringViewTest,RFind) -- cgit v1.2.3-70-g09d2 From f7e6eaf84bebac565f462e2ce48f30808cc771eb Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 8 Jun 2020 17:07:43 +0200 Subject: Avoid undefined pointer arithmetic while growing mmap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The undefined behaviour sanitizer complains with: runtime error: addition of unsigned offset to 0x… overflowed to 0x… Compilers and runtime do the right thing in any case and it is a codepath that can (and ideally should) be avoided for speed reasons alone, but fixing it can't hurt (too much). --- apt-pkg/cacheiterators.h | 8 +++--- apt-pkg/contrib/mmap.cc | 2 +- apt-pkg/pkgcachegen.cc | 63 ++++++++++++++++++------------------------------ apt-pkg/pkgcachegen.h | 2 +- 4 files changed, 29 insertions(+), 46 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/cacheiterators.h b/apt-pkg/cacheiterators.h index 6261b5089..466492442 100644 --- a/apt-pkg/cacheiterators.h +++ b/apt-pkg/cacheiterators.h @@ -82,10 +82,10 @@ template class APT_PUBLIC pkgCache::Iterator : inline unsigned long Index() const {return S - OwnerPointer();} inline map_pointer MapPointer() const {return map_pointer(Index()) ;} - void ReMap(void const * const oldMap, void const * const newMap) { + void ReMap(void const * const oldMap, void * const newMap) { if (Owner == 0 || S == 0) return; - S += static_cast(newMap) - static_cast(oldMap); + S = static_cast(newMap) + (S - static_cast(oldMap)); } // Constructors - look out for the variable assigning @@ -350,12 +350,12 @@ class APT_PUBLIC pkgCache::DepIterator : public Iterator() const {return (DependencyProxy) { S2->Version, S2->Package, S->ID, S2->Type, S2->CompareOp, S->ParentVer, S->DependencyData, S->NextRevDepends, S->NextDepends, S2->NextData };} inline DependencyProxy operator->() {return (DependencyProxy) { S2->Version, S2->Package, S->ID, S2->Type, S2->CompareOp, S->ParentVer, S->DependencyData, S->NextRevDepends, S->NextDepends, S2->NextData };} - void ReMap(void const * const oldMap, void const * const newMap) + void ReMap(void const * const oldMap, void * const newMap) { Iterator::ReMap(oldMap, newMap); if (Owner == 0 || S == 0 || S2 == 0) return; - S2 += static_cast(newMap) - static_cast(oldMap); + S2 = static_cast(newMap) + (S2 - static_cast(oldMap)); } //Nice printable representation diff --git a/apt-pkg/contrib/mmap.cc b/apt-pkg/contrib/mmap.cc index 020491172..642e20473 100644 --- a/apt-pkg/contrib/mmap.cc +++ b/apt-pkg/contrib/mmap.cc @@ -393,7 +393,7 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize) bool const newError = _error->PendingError(); _error->MergeWithStack(); if (Pools != oldPools) - I += Pools - oldPools; + I = Pools + (I - oldPools); // Does the allocation failed ? if (Result == 0 && newError) diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 26cf7fc68..6f2e3268c 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -156,13 +156,14 @@ pkgCacheGenerator::~pkgCacheGenerator() Map.Sync(0,sizeof(pkgCache::Header)); } /*}}}*/ -void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap, size_t oldSize) {/*{{{*/ +void pkgCacheGenerator::ReMap(void const * const oldMap, void * const newMap, size_t oldSize) {/*{{{*/ + if (oldMap == newMap) + return; + // Prevent multiple remaps of the same iterator. If seen.insert(iterator) // returns (something, true) the iterator was not yet seen and we can // remap it. std::unordered_set seen; - if (oldMap == newMap) - return; if (_config->FindB("Debug::pkgCacheGen", false)) std::clog << "Remapping from " << oldMap << " to " << newMap << std::endl; @@ -170,42 +171,24 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM Cache.ReMap(false); if (CurrentFile != nullptr) - CurrentFile += static_cast(newMap) - static_cast(oldMap); + CurrentFile = static_cast(newMap) + (CurrentFile - static_cast(oldMap)); if (CurrentRlsFile != nullptr) - CurrentRlsFile += static_cast(newMap) - static_cast(oldMap); - - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); - for (std::vector::const_iterator i = Dynamic::toReMap.begin(); - i != Dynamic::toReMap.end(); ++i) - if (std::get<1>(seen.insert(*i)) == true) - (*i)->ReMap(oldMap, newMap); + CurrentRlsFile = static_cast(newMap) + (CurrentRlsFile - static_cast(oldMap)); + +#define APT_REMAP(TYPE) \ + for (auto * const i : Dynamic::toReMap) \ + if (seen.insert(i).second) \ + i->ReMap(oldMap, newMap) + APT_REMAP(pkgCache::GrpIterator); + APT_REMAP(pkgCache::PkgIterator); + APT_REMAP(pkgCache::VerIterator); + APT_REMAP(pkgCache::DepIterator); + APT_REMAP(pkgCache::DescIterator); + APT_REMAP(pkgCache::PrvIterator); + APT_REMAP(pkgCache::PkgFileIterator); + APT_REMAP(pkgCache::RlsFileIterator); +#undef APT_REMAP + for (APT::StringView* ViewP : Dynamic::toReMap) { if (std::get<1>(seen.insert(ViewP)) == false) continue; @@ -453,7 +436,7 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator Pkg.Name(), "NewVersion", 1); if (oldMap != Map.Data()) - LastVer += static_cast const *>(Map.Data()) - static_cast const *>(oldMap); + LastVer = static_cast *>(Map.Data()) + (LastVer - static_cast const *>(oldMap)); *LastVer = verindex; if (unlikely(List.NewVersion(Ver) == false)) @@ -1073,7 +1056,7 @@ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, for (pkgCache::DepIterator D = Ver.DependsList(); D.end() == false; ++D) OldDepLast = &D->NextDepends; } else if (oldMap != Map.Data()) - OldDepLast += static_cast const *>(Map.Data()) - static_cast const *>(oldMap); + OldDepLast = static_cast *>(Map.Data()) + (OldDepLast - static_cast const *>(oldMap)); Dep->NextDepends = *OldDepLast; *OldDepLast = Dependency; diff --git a/apt-pkg/pkgcachegen.h b/apt-pkg/pkgcachegen.h index 62c171be4..f4781d715 100644 --- a/apt-pkg/pkgcachegen.h +++ b/apt-pkg/pkgcachegen.h @@ -146,7 +146,7 @@ class APT_HIDDEN pkgCacheGenerator /*{{{*/ MMap **OutMap,pkgCache **OutCache, bool AllowMem = false); APT_PUBLIC static bool MakeOnlyStatusCache(OpProgress *Progress,DynamicMMap **OutMap); - void ReMap(void const * const oldMap, void const * const newMap, size_t oldSize); + void ReMap(void const * const oldMap, void * const newMap, size_t oldSize); bool Start(); pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress); -- cgit v1.2.3-70-g09d2 From 96dc40b19623621a9cc2c5541fb3adbbceb553b1 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 28 Jun 2020 20:52:09 +0200 Subject: Replace PrintStatus with SendMessage usage varg API is a nightmare as the symbols seems different on ever other arch, but more importantly SendMessage does a few checks on the content of the message and it is all outputted via C++ iostreams and not mixed in FILE* which is handy for overriding the streams. --- apt-pkg/acquire-method.cc | 44 ++++++++++++++++++++++++++++++++++++++------ apt-pkg/acquire-method.h | 2 +- apt-pkg/contrib/gpgv.cc | 15 --------------- apt-pkg/contrib/strutl.cc | 11 +++++------ apt-pkg/contrib/strutl.h | 2 ++ methods/aptmethod.h | 30 ++++++++++++++++++------------ methods/basehttp.cc | 4 +++- methods/gpgv.cc | 8 ++++++-- 8 files changed, 73 insertions(+), 43 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-method.cc b/apt-pkg/acquire-method.cc index 6e1674f7f..089582561 100644 --- a/apt-pkg/acquire-method.cc +++ b/apt-pkg/acquire-method.cc @@ -482,9 +482,25 @@ void pkgAcqMethod::PrintStatus(char const * const header, const char* Format, void pkgAcqMethod::Log(const char *Format,...) { va_list args; - va_start(args,Format); - PrintStatus("101 Log", Format, args); - va_end(args); + ssize_t size = 400; + std::ostringstream outstr; + while (true) { + bool ret; + va_start(args,Format); + ret = iovprintf(outstr, Format, args, size); + va_end(args); + if (ret == true) + break; + } + std::unordered_map fields; + if (Queue != 0) + try_emplace(fields, "URI", Queue->Uri); + else + try_emplace(fields, "URI", ""); + if (not UsedMirror.empty()) + try_emplace(fields, "UsedMirror", UsedMirror); + try_emplace(fields, "Message", outstr.str()); + SendMessage("101 Log", std::move(fields)); } /*}}}*/ // AcqMethod::Status - Send a status message /*{{{*/ @@ -493,9 +509,25 @@ void pkgAcqMethod::Log(const char *Format,...) void pkgAcqMethod::Status(const char *Format,...) { va_list args; - va_start(args,Format); - PrintStatus("102 Status", Format, args); - va_end(args); + ssize_t size = 400; + std::ostringstream outstr; + while (true) { + bool ret; + va_start(args,Format); + ret = iovprintf(outstr, Format, args, size); + va_end(args); + if (ret == true) + break; + } + std::unordered_map fields; + if (Queue != 0) + try_emplace(fields, "URI", Queue->Uri); + else + try_emplace(fields, "URI", ""); + if (not UsedMirror.empty()) + try_emplace(fields, "UsedMirror", UsedMirror); + try_emplace(fields, "Message", outstr.str()); + SendMessage("102 Status", std::move(fields)); } /*}}}*/ // AcqMethod::Redirect - Send a redirect message /*{{{*/ diff --git a/apt-pkg/acquire-method.h b/apt-pkg/acquire-method.h index e854f5ff1..b4b238c4c 100644 --- a/apt-pkg/acquire-method.h +++ b/apt-pkg/acquire-method.h @@ -101,7 +101,7 @@ class APT_PUBLIC pkgAcqMethod bool MediaFail(std::string Required,std::string Drive); virtual void Exit() {}; - void PrintStatus(char const * const header, const char* Format, va_list &args) const; + APT_DEPRECATED_MSG("Use SendMessage instead") void PrintStatus(char const * const header, const char* Format, va_list &args) const; public: enum CnfFlags diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 28f3150c3..3368ece84 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -121,21 +121,6 @@ static bool operator!=(LineBuffer const &buf, APT::StringView const exp) noexcep And as a cherry on the cake, we use our apt-key wrapper to do part of the lifting in regards to merging keyrings. Fun for the whole family. */ -static bool iovprintf(std::ostream &out, const char *format, - va_list &args, ssize_t &size) { - auto S = make_unique_char(malloc(size)); - ssize_t const n = vsnprintf(S.get(), size, format, args); - if (n > -1 && n < size) { - out << S.get(); - return true; - } else { - if (n > -1) - size = n + 1; - else - size *= 2; - } - return false; -} static void APT_PRINTF(4) apt_error(std::ostream &outterm, int const statusfd, int fd[2], const char *format, ...) { std::ostringstream outstr; diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 84e79122e..3a0a6eaa3 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1414,13 +1415,12 @@ unsigned long RegexChoice(RxChoiceList *Rxs,const char **ListBegin, // --------------------------------------------------------------------- /* This is used to make the internationalization strings easier to translate and to allow reordering of parameters */ -static bool iovprintf(ostream &out, const char *format, +bool iovprintf(std::ostream &out, const char *format, va_list &args, ssize_t &size) { - char *S = (char*)malloc(size); - ssize_t const n = vsnprintf(S, size, format, args); + auto S = std::unique_ptr{static_cast(malloc(size)), &free}; + ssize_t const n = vsnprintf(S.get(), size, format, args); if (n > -1 && n < size) { - out << S; - free(S); + out << S.get(); return true; } else { if (n > -1) @@ -1428,7 +1428,6 @@ static bool iovprintf(ostream &out, const char *format, else size *= 2; } - free(S); return false; } void ioprintf(ostream &out,const char *format,...) diff --git a/apt-pkg/contrib/strutl.h b/apt-pkg/contrib/strutl.h index 8669c97a5..b6e6bfdce 100644 --- a/apt-pkg/contrib/strutl.h +++ b/apt-pkg/contrib/strutl.h @@ -117,6 +117,8 @@ APT_PUBLIC std::vector StringSplit(std::string const &input, std::string const &sep, unsigned int maxsplit=std::numeric_limits::max()) APT_PURE; + +APT_HIDDEN bool iovprintf(std::ostream &out, const char *format, va_list &args, ssize_t &size); APT_PUBLIC void ioprintf(std::ostream &out,const char *format,...) APT_PRINTF(2); APT_PUBLIC void strprintf(std::string &out,const char *format,...) APT_PRINTF(2); APT_PUBLIC char *safe_snprintf(char *Buffer,char *End,const char *Format,...) APT_PRINTF(3); diff --git a/methods/aptmethod.h b/methods/aptmethod.h index 7038131cf..bd50e8078 100644 --- a/methods/aptmethod.h +++ b/methods/aptmethod.h @@ -325,7 +325,11 @@ protected: rc = seccomp_load(ctx); if (rc == -EINVAL) - Warning("aptMethod::Configuration: could not load seccomp policy: %s", strerror(-rc)); + { + std::string msg; + strprintf(msg, "aptMethod::Configuration: could not load seccomp policy: %s", strerror(-rc)); + Warning(std::move(msg)); + } else if (rc != 0) return _error->FatalE("aptMethod::Configuration", "could not load seccomp policy: %s", strerror(-rc)); @@ -380,12 +384,17 @@ protected: return true; } - void Warning(const char *Format,...) + void Warning(std::string &&msg) { - va_list args; - va_start(args,Format); - PrintStatus("104 Warning", Format, args); - va_end(args); + std::unordered_map fields; + if (Queue != 0) + fields.emplace("URI", Queue->Uri); + else + fields.emplace("URI", ""); + if (not UsedMirror.empty()) + fields.emplace("UsedMirror", UsedMirror); + fields.emplace("Message", std::move(msg)); + SendMessage("104 Warning", std::move(fields)); } std::vector methodNames; @@ -560,14 +569,11 @@ class aptAuthConfMethod : public aptMethod result &= MaybeAddAuth(*authconf, uri); } - if (not _error->empty()) + while (not _error->empty()) { std::string message; - while (not _error->empty()) - { - _error->PopMessage(message); - Warning("%s", message.c_str()); - } + _error->PopMessage(message); + Warning(std::move(message)); } _error->RevertToStack(); diff --git a/methods/basehttp.cc b/methods/basehttp.cc index 8aac1090c..b75b450cc 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -762,7 +762,9 @@ int BaseHttpMethod::Loop() // yes, he did! Disable pipelining and rewrite queue if (Server->Pipeline == true) { - Warning(_("Automatically disabled %s due to incorrect response from server/proxy. (man 5 apt.conf)"), "Acquire::http::Pipeline-Depth"); + std::string msg; + strprintf(msg, _("Automatically disabled %s due to incorrect response from server/proxy. (man 5 apt.conf)"), "Acquire::http::Pipeline-Depth"); + Warning(std::move(msg)); Server->Pipeline = false; Server->PipelineAllowed = false; // we keep the PipelineDepth value so that the rest of the queue can be fixed up as well diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 08d030a17..a9da456ec 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -264,7 +264,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, SubKeyMapping[tokens[9]].emplace_back(sig); } else if (strncmp(buffer, APTKEYWARNING, sizeof(APTKEYWARNING)-1) == 0) - Warning("%s", buffer + sizeof(APTKEYWARNING)); + Warning(buffer + sizeof(APTKEYWARNING)); else if (strncmp(buffer, APTKEYERROR, sizeof(APTKEYERROR)-1) == 0) _error->Error("%s", buffer + sizeof(APTKEYERROR)); } @@ -442,8 +442,12 @@ bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm) })) { for (auto const & Signer : Signers.SoonWorthless) + { + std::string msg; // TRANSLATORS: The second %s is the reason and is untranslated for repository owners. - Warning(_("Signature by key %s uses weak digest algorithm (%s)"), Signer.key.c_str(), Signer.note.c_str()); + strprintf(msg, _("Signature by key %s uses weak digest algorithm (%s)"), Signer.key.c_str(), Signer.note.c_str()); + Warning(std::move(msg)); + } } if (Signers.Good.empty() || !Signers.Bad.empty() || !Signers.NoPubKey.empty()) -- cgit v1.2.3-70-g09d2 From c31ba27ecf7e35e03af34c3d74d3c6c93976f89c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 21:47:00 +0100 Subject: Limit on first patch size only for server-merged patches APT tries to detect if applying patches is more costly than just downloading the complete index by combining the size of the patches. That is correct for client-side merging, but for server-side merging we actually don't know if we will jump directly from old to current or have still intermediate steps in between. With this commit we assume it will be a jump from old to current through as that is what dak implements and it seems reasonable if you go to the trouble of server side merging that the server does the entire merging in one file instead of leaving additional work for the client to do. Note that this just changes the heuristic to prevent apt from discarding patches as uneconomic in the now more common one merged-patch style, it still supports multiple merged patches as before. To resolve this cleanly we would need another field in the index file declaring which hash we will arrive at if a patch is applied (or a field differentiating between these merged-patch styles at least), but that seems like overkill for now. --- apt-pkg/acquire-item.cc | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index a4ad6b854..c9e81070b 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -2604,14 +2604,32 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/ return false; } + /* decide if we should download patches one by one or in one go: + The first is good if the server merges patches, but many don't so client + based merging can be attempt in which case the second is better. + "bad things" will happen if patches are merged on the server, + but client side merging is attempt as well */ + pdiff_merge = _config->FindB("Acquire::PDiffs::Merge", true); + if (pdiff_merge == true) + { + // reprepro and dak add this flag if they merge patches on the server + std::string const precedence = Tags.FindS("X-Patch-Precedence"); + pdiff_merge = (precedence != "merged"); + } + // calculate the size of all patches we have to get unsigned short const sizeLimitPercent = _config->FindI("Acquire::PDiffs::SizeLimit", 100); if (sizeLimitPercent > 0) { - unsigned long long downloadSize = std::accumulate(available_patches.begin(), - available_patches.end(), 0llu, [](unsigned long long const T, DiffInfo const &I) { - return T + I.download_hashes.FileSize(); - }); + unsigned long long downloadSize = 0; + if (pdiff_merge) + downloadSize = std::accumulate(available_patches.begin(), available_patches.end(), 0llu, + [](unsigned long long const T, DiffInfo const &I) { + return T + I.download_hashes.FileSize(); + }); + // if server-side merging, assume we will need only the first patch + else if (not available_patches.empty()) + downloadSize = available_patches.front().download_hashes.FileSize(); if (downloadSize != 0) { unsigned long long downloadSizeIdx = 0; @@ -2636,19 +2654,6 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/ } } - /* decide if we should download patches one by one or in one go: - The first is good if the server merges patches, but many don't so client - based merging can be attempt in which case the second is better. - "bad things" will happen if patches are merged on the server, - but client side merging is attempt as well */ - pdiff_merge = _config->FindB("Acquire::PDiffs::Merge", true); - if (pdiff_merge == true) - { - // reprepro adds this flag if it has merged patches on the server - std::string const precedence = Tags.FindS("X-Patch-Precedence"); - pdiff_merge = (precedence != "merged"); - } - // clean the plate { std::string const Final = GetExistingFilename(CurrentPackagesFile); -- cgit v1.2.3-70-g09d2 From a158e78152316f56b1a324a42a2f00f0a8662ac3 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 22:41:56 +0100 Subject: Use size of the old cache as APT::Cache-Start default Depending on your configured source 25 MB is hardly enough, so the mmap housing the cache while it is build has to grow. Repeatedly. We can cut down on the repeats of this by keeping a record of the size of the old cache assuming the sizes will remain roughly in the same ballpark. --- apt-pkg/cachefile.cc | 23 +++++++++++++++++++---- apt-pkg/pkgcachegen.cc | 11 ++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc index 8b86fa3e4..5355994d3 100644 --- a/apt-pkg/cachefile.cc +++ b/apt-pkg/cachefile.cc @@ -27,10 +27,13 @@ #include #include +#include #include #include #include #include +#include +#include #include #include @@ -288,15 +291,27 @@ bool pkgCacheFile::AddIndexFile(pkgIndexFile * const File) /*{{{*/ // CacheFile::RemoveCaches - remove all cache files from disk /*{{{*/ // --------------------------------------------------------------------- /* */ +static void SetCacheStartBeforeRemovingCache(std::string const &cache) +{ + if (cache.empty()) + return; + auto const CacheStart = _config->FindI("APT::Cache-Start", 0); + constexpr auto CacheStartDefault = 24 * 1024 * 1024; + struct stat Buf; + if (stat(cache.c_str(), &Buf) == 0 && (Buf.st_mode & S_IFREG) != 0) + { + RemoveFile("RemoveCaches", cache); + if (CacheStart == 0 && std::numeric_limits::max() >= Buf.st_size && Buf.st_size > CacheStartDefault) + _config->Set("APT::Cache-Start", Buf.st_size); + } +} void pkgCacheFile::RemoveCaches() { std::string const pkgcache = _config->FindFile("Dir::cache::pkgcache"); + SetCacheStartBeforeRemovingCache(pkgcache); std::string const srcpkgcache = _config->FindFile("Dir::cache::srcpkgcache"); + SetCacheStartBeforeRemovingCache(srcpkgcache); - if (pkgcache.empty() == false && RealFileExists(pkgcache) == true) - RemoveFile("RemoveCaches", pkgcache); - if (srcpkgcache.empty() == false && RealFileExists(srcpkgcache) == true) - RemoveFile("RemoveCaches", srcpkgcache); if (pkgcache.empty() == false) { std::string cachedir = flNotFile(pkgcache); diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 6f2e3268c..b4fd0641e 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -37,6 +37,8 @@ #include /*}}}*/ +constexpr auto APT_CACHE_START_DEFAULT = 24 * 1024 * 1024; + template using Dynamic = pkgCacheGenerator::Dynamic; typedef std::vector::iterator FileIterator; template std::vector pkgCacheGenerator::Dynamic::toReMap; @@ -1383,6 +1385,13 @@ static bool CheckValidity(FileFd &CacheFile, std::string const &CacheFileName, return false; } + if (_config->FindI("APT::Cache-Start", 0) == 0) + { + auto const size = CacheFile.FileSize(); + if (std::numeric_limits::max() >= size && size > APT_CACHE_START_DEFAULT) + _config->Set("APT::Cache-Start", size); + } + if (List.GetLastModifiedTime() > CacheFile.ModificationTime()) { if (Debug == true) @@ -1591,7 +1600,7 @@ static bool BuildCache(pkgCacheGenerator &Gen, where it builds the cache 'fast' into a memory buffer. */ static DynamicMMap* CreateDynamicMMap(FileFd * const CacheF, unsigned long Flags) { - map_filesize_t const MapStart = _config->FindI("APT::Cache-Start", 24*1024*1024); + map_filesize_t const MapStart = _config->FindI("APT::Cache-Start", APT_CACHE_START_DEFAULT); map_filesize_t const MapGrow = _config->FindI("APT::Cache-Grow", 1*1024*1024); map_filesize_t const MapLimit = _config->FindI("APT::Cache-Limit", 0); Flags |= MMap::Moveable; -- cgit v1.2.3-70-g09d2