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(-) 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-18-g5258 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(-) 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-18-g5258 From c8d65a59a36400aaf85ff96b0c813f8fc53cd8bb Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 15:26:32 +0100 Subject: Remove unused config check for pre-XXH3 cache hashing References: 1460eebf2abe913df964e031eff081a57f043697 Gbp-Dch: Ignore --- CMake/CheckCxxTarget.cmake | 36 ------------------------------------ CMake/config.h.in | 4 ---- CMakeLists.txt | 5 ----- 3 files changed, 45 deletions(-) delete mode 100644 CMake/CheckCxxTarget.cmake diff --git a/CMake/CheckCxxTarget.cmake b/CMake/CheckCxxTarget.cmake deleted file mode 100644 index 17c32bfac..000000000 --- a/CMake/CheckCxxTarget.cmake +++ /dev/null @@ -1,36 +0,0 @@ -# CMake support for target-based function multiversioning -# -# Copyright (C) 2019 Canonical Ltd -# -# Author: Julian Andres Klode . -# -# Permission is hereby granted, free of charge, to any person -# obtaining a copy of this software and associated documentation files -# (the "Software"), to deal in the Software without restriction, -# including without limitation the rights to use, copy, modify, merge, -# publish, distribute, sublicense, and/or sell copies of the Software, -# and to permit persons to whom the Software is furnished to do so, -# subject to the following conditions: -# -# The above copyright notice and this permission notice shall be -# included in all copies or substantial portions of the Software. -# -# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, -# EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF -# MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND -# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS -# BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN -# ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN -# CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -# SOFTWARE. - - -include(CheckCXXSourceCompiles) -function(check_cxx_target var target code) - check_cxx_source_compiles( - " - __attribute__((target(\"${target}\"))) static int foo(int i) { return ${code}; } - __attribute__((target(\"default\"))) static int foo(int i) { return i; } - int main(int i, char **) { return foo(i); } - " ${var}) -endfunction() diff --git a/CMake/config.h.in b/CMake/config.h.in index 911d42464..8fa5e8b6f 100644 --- a/CMake/config.h.in +++ b/CMake/config.h.in @@ -80,9 +80,5 @@ /* Group of the root user */ #cmakedefine ROOT_GROUP "${ROOT_GROUP}" -/* defined if __builtin_ia32_crc32{s,d}i() exists in an sse4.2 target */ -#cmakedefine HAVE_FMV_SSE42_AND_CRC32 -#cmakedefine HAVE_FMV_SSE42_AND_CRC32DI - /* unrolling is faster combined with an optimizing compiler */ #define SHA2_UNROLL_TRANSFORM diff --git a/CMakeLists.txt b/CMakeLists.txt index 2d9afba61..9e35f6e00 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -197,11 +197,6 @@ else() set(RESOLV_LIBRARIES -lresolv) endif() -# Check multiversioning -include(CheckCxxTarget) -check_cxx_target(HAVE_FMV_SSE42_AND_CRC32 "sse4.2" "__builtin_ia32_crc32si(0,i)|__builtin_ia32_crc32hi(0,i)|__builtin_ia32_crc32qi(0,i)") -check_cxx_target(HAVE_FMV_SSE42_AND_CRC32DI "sse4.2" "__builtin_ia32_crc32di(0,i)") - # Configure some variables like package, version and architecture. set(PACKAGE ${PROJECT_NAME}) set(PACKAGE_MAIL "APT Development Team ") -- cgit v1.2.3-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 From 1b3ad3891465f8b72bcedfb4edd513cb74eec7f3 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:35:29 +0100 Subject: Add a simple test for APT::String::DisplayLength References: 2497198e9599a6a8d4d0ad08627bcfc7ea49c644 Gbp-Dch: Ignore --- test/libapt/strutil_test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index b7132c35f..349946b5f 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -361,3 +361,20 @@ TEST(StrUtilTest, LookupTag) EXPECT_EQ("Value4", LookupTag(msg, "Field4", "")); EXPECT_EQ("Value5", LookupTag(msg, "Field5", "")); } + +TEST(StrUtilTest, DisplayLength) +{ + EXPECT_EQ(0, APT::String::DisplayLength("")); + EXPECT_EQ(1, APT::String::DisplayLength("a")); + EXPECT_EQ(3, APT::String::DisplayLength("apt")); + EXPECT_EQ(1, APT::String::DisplayLength("@")); + EXPECT_EQ(3, APT::String::DisplayLength("き")); + + EXPECT_EQ(1, APT::String::DisplayLength("$")); + EXPECT_EQ(2, APT::String::DisplayLength("¢")); + EXPECT_EQ(3, APT::String::DisplayLength("ह")); + EXPECT_EQ(3, APT::String::DisplayLength("€")); + EXPECT_EQ(3, APT::String::DisplayLength("한")); + EXPECT_EQ(4, APT::String::DisplayLength("𐍈")); + EXPECT_EQ(16, APT::String::DisplayLength("𐍈한€ह¢$")); +} -- cgit v1.2.3-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 From 35af71bc026d85aef4af979aa247e837d91dfc1c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 17:50:05 +0100 Subject: Use error reporting instead of assert in rred patching The rest of our code uses return-code errors and it isn't that nice to crash the rred method on bad patches anyway, so we move to properly reporting errors in our usual way which incidently also helps writing a fuzzer for it. This is not really security relevant though as the patches passed hash verification while they were downloaded, so an attacker has to overtake a trusted repository first which gives plenty better options of attack. --- methods/rred.cc | 156 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 88 insertions(+), 68 deletions(-) diff --git a/methods/rred.cc b/methods/rred.cc index 981364a9e..f53f05ad5 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -7,12 +7,15 @@ #include +#ifndef APT_EXCLUDE_RRED_METHOD_CODE #include "aptmethod.h" #include +#include +#endif + #include #include #include -#include #include #include @@ -23,7 +26,7 @@ #include #include -#include +#include #include #include #include @@ -33,7 +36,9 @@ #include -#define BLOCK_SIZE (512*1024) +#ifndef APT_MEMBLOCK_SIZE +#define APT_MEMBLOCK_SIZE (512*1024) +#endif static bool ShowHelp(CommandLine &) { @@ -66,9 +71,9 @@ class MemBlock { char *start; size_t size; char *free; - MemBlock *next; + MemBlock *next = nullptr; - explicit MemBlock(size_t size) : size(size), next(NULL) + explicit MemBlock(size_t size) : size(size) { free = start = new char[size]; } @@ -77,11 +82,7 @@ class MemBlock { public: - MemBlock(void) { - free = start = new char[BLOCK_SIZE]; - size = BLOCK_SIZE; - next = NULL; - } + MemBlock() : MemBlock(APT_MEMBLOCK_SIZE) {} ~MemBlock() { delete [] start; @@ -100,11 +101,9 @@ class MemBlock { for (MemBlock *k = this; k; k = k->next) { if (k->free == last) { if (len <= k->avail()) { - char *n = k->add(src, len); - assert(last == n); - if (last == n) - return NULL; - return n; + char * const n = k->add(src, len); + assert(last == n); // we checked already that the block is big enough, so a new one shouldn't be used + return (last == n) ? nullptr : n; } else { break; } @@ -119,10 +118,10 @@ class MemBlock { char *add(char *src, size_t len) { if (len > avail()) { if (!next) { - if (len > BLOCK_SIZE) { + if (len > APT_MEMBLOCK_SIZE) { next = new MemBlock(len); } else { - next = new MemBlock; + next = new MemBlock(); } } return next->add(src, len); @@ -155,24 +154,27 @@ struct Change { } /* actually, don't write lines from */ - void skip_lines(size_t lines) + bool skip_lines(size_t lines) { while (lines > 0) { char *s = (char*) memchr(add, '\n', add_len); - assert(s != NULL); + if (s == nullptr) + return _error->Error("No line left in add_len data to skip (1)"); s++; add_len -= (s - add); add_cnt--; lines--; if (add_len == 0) { - add = NULL; - assert(add_cnt == 0); - assert(lines == 0); + add = nullptr; + if (add_cnt != 0 || lines != 0) + return _error->Error("No line left in add_len data to skip (2)"); } else { add = s; - assert(add_cnt > 0); + if (add_cnt == 0) + return _error->Error("No line left in add_len data to skip (3)"); } } + return true; } }; @@ -183,7 +185,8 @@ class FileChanges { bool pos_is_okay(void) const { -#ifdef POSDEBUG +#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION + // this isn't unsafe, it is just a moderately expensive check we want to avoid normally size_t cpos = 0; std::list::const_iterator x; for (x = changes.begin(); x != where; ++x) { @@ -208,33 +211,40 @@ class FileChanges { std::list::reverse_iterator rbegin(void) { return changes.rbegin(); } std::list::reverse_iterator rend(void) { return changes.rend(); } - void add_change(Change c) { + bool add_change(Change c) { assert(pos_is_okay()); - go_to_change_for(c.offset); - assert(pos + where->offset == c.offset); + if (not go_to_change_for(c.offset) || + pos + where->offset != c.offset) + return false; if (c.del_cnt > 0) - delete_lines(c.del_cnt); - assert(pos + where->offset == c.offset); + if (not delete_lines(c.del_cnt)) + return false; + if (pos + where->offset != c.offset) + return false; if (c.add_len > 0) { assert(pos_is_okay()); if (where->add_len > 0) - new_change(); - assert(where->add_len == 0 && where->add_cnt == 0); + if (not new_change()) + return false; + if (where->add_len != 0 || where->add_cnt != 0) + return false; where->add_len = c.add_len; where->add_cnt = c.add_cnt; where->add = c.add; } assert(pos_is_okay()); - merge(); - assert(pos_is_okay()); + if (not merge()) + return false; + return pos_is_okay(); } private: - void merge(void) + bool merge(void) { while (where->offset == 0 && where != changes.begin()) { - left(); + if (not left()) + return false; } std::list::iterator next = where; ++next; @@ -253,53 +263,56 @@ class FileChanges { ++next; } } + return true; } - void go_to_change_for(size_t line) + bool go_to_change_for(size_t line) { while(where != changes.end()) { if (line < pos) { - left(); + if (not left()) + return false; continue; } if (pos + where->offset + where->add_cnt <= line) { - right(); + if (not right()) + return false; continue; } // line is somewhere in this slot if (line < pos + where->offset) { break; } else if (line == pos + where->offset) { - return; + return true; } else { - split(line - pos); - right(); - return; + if (not split(line - pos)) + return false; + return right(); } } /* it goes before this patch */ - insert(line-pos); + return insert(line-pos); } - void new_change(void) { insert(where->offset); } + bool new_change(void) { return insert(where->offset); } - void insert(size_t offset) + bool insert(size_t offset) { assert(pos_is_okay()); - assert(where == changes.end() || offset <= where->offset); + if (where != changes.end() && offset > where->offset) + return false; if (where != changes.end()) where->offset -= offset; changes.insert(where, Change(offset)); --where; - assert(pos_is_okay()); + return pos_is_okay(); } - void split(size_t offset) + bool split(size_t offset) { assert(pos_is_okay()); - - assert(where->offset < offset); - assert(offset < where->offset + where->add_cnt); + if (where->offset >= offset || offset >= where->offset + where->add_cnt) + return false; size_t keep_lines = offset - where->offset; @@ -307,27 +320,29 @@ class FileChanges { where->del_cnt = 0; where->offset = 0; - where->skip_lines(keep_lines); + if (not where->skip_lines(keep_lines)) + return false; before.add_cnt = keep_lines; before.add_len -= where->add_len; changes.insert(where, before); --where; - assert(pos_is_okay()); + return pos_is_okay(); } - void delete_lines(size_t cnt) + bool delete_lines(size_t cnt) { - std::list::iterator x = where; assert(pos_is_okay()); + std::list::iterator x = where; while (cnt > 0) { size_t del; del = x->add_cnt; if (del > cnt) del = cnt; - x->skip_lines(del); + if (not x->skip_lines(del)) + return false; cnt -= del; ++x; @@ -342,21 +357,21 @@ class FileChanges { where->del_cnt += del; cnt -= del; } - assert(pos_is_okay()); + return pos_is_okay(); } - void left(void) { + bool left(void) { assert(pos_is_okay()); --where; pos -= where->offset + where->add_cnt; - assert(pos_is_okay()); + return pos_is_okay(); } - void right(void) { + bool right(void) { assert(pos_is_okay()); pos += where->offset + where->add_cnt; ++where; - assert(pos_is_okay()); + return pos_is_okay(); } }; @@ -378,7 +393,7 @@ class Patch { static void dump_rest(FileFd &o, FileFd &i, Hashes * const start_hash, Hashes * const end_hash) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; unsigned long long l = 0; while (i.Read(buffer, sizeof(buffer), &l)) { if (l ==0 || !retry_fwrite(buffer, l, o, start_hash, end_hash)) @@ -389,7 +404,7 @@ class Patch { static void dump_lines(FileFd &o, FileFd &i, size_t n, Hashes * const start_hash, Hashes * const end_hash) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; while (n > 0) { if (i.ReadLine(buffer, sizeof(buffer)) == NULL) buffer[0] = '\0'; @@ -402,7 +417,7 @@ class Patch { static void skip_lines(FileFd &i, int n, Hashes * const start_hash) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; while (n > 0) { if (i.ReadLine(buffer, sizeof(buffer)) == NULL) buffer[0] = '\0'; @@ -422,7 +437,7 @@ class Patch { bool read_diff(FileFd &f, Hashes * const h) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; bool cmdwanted = true; Change ch(std::numeric_limits::max()); @@ -478,7 +493,8 @@ class Patch { ch.add = NULL; ch.add_cnt = 0; ch.add_len = 0; - filechanges.add_change(ch); + if (not filechanges.add_change(ch)) + return _error->Error("Parsing patchfile %s failed: Delete command could not be added to changes", f.Name().c_str()); break; default: return _error->Error("Parsing patchfile %s failed: Unknown command", f.Name().c_str()); @@ -486,7 +502,8 @@ class Patch { } else { /* !cmdwanted */ if (strcmp(buffer, ".\n") == 0) { cmdwanted = true; - filechanges.add_change(ch); + if (not filechanges.add_change(ch)) + return _error->Error("Parsing patchfile %s failed: Data couldn't be added for command (1)", f.Name().c_str()); } else { char *last = NULL; char *add; @@ -500,7 +517,8 @@ class Patch { ch.add_cnt++; } else { if (ch.add) { - filechanges.add_change(ch); + if (not filechanges.add_change(ch)) + return _error->Error("Parsing patchfile %s failed: Data couldn't be added for command (2)", f.Name().c_str()); ch.del_cnt = 0; } ch.offset += ch.add_cnt; @@ -575,6 +593,7 @@ class Patch { } }; +#ifndef APT_EXCLUDE_RRED_METHOD_CODE class RredMethod : public aptMethod { private: bool Debug; @@ -859,3 +878,4 @@ int main(int argc, const char *argv[]) input.Close(); return DispatchCommandLine(CmdL, {}); } +#endif -- cgit v1.2.3-18-g5258 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(-) 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-18-g5258 From 3e53dbbe758a4e2da378ebf0296d8105d4a5804c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 28 Jun 2020 20:59:27 +0200 Subject: Ensure HTTP status code text has sensible content We use the code in error messages, so at least for that edgecase we should ensure that it has sensible content. Note that the acquire system aborts on non-sensible message content in SendMessage, so you can't really exploit this. --- methods/basehttp.cc | 3 +++ methods/basehttp.h | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/methods/basehttp.cc b/methods/basehttp.cc index b75b450cc..3786e2e6c 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -110,6 +110,9 @@ bool RequestState::HeaderLine(string const &Line) /*{{{*/ if (sscanf(Line.c_str(),"HTTP %3u%359[^\n]",&Result,Code) != 2) return _error->Error(_("The HTTP server sent an invalid reply header")); } + auto const CodeLen = strlen(Code); + auto const CodeEnd = std::remove_if(Code, Code + CodeLen, [](char c) { return isprint(c) == 0; }); + *CodeEnd = '\0'; /* Check the HTTP response header to get the default persistence state. */ diff --git a/methods/basehttp.h b/methods/basehttp.h index 62c9963ea..c0d14d854 100644 --- a/methods/basehttp.h +++ b/methods/basehttp.h @@ -60,7 +60,7 @@ struct RequestState bool AddPartialFileToHashes(FileFd &File); RequestState(BaseHttpMethod * const Owner, ServerState * const Server) : - Owner(Owner), Server(Server) { time(&Date); } + Owner(Owner), Server(Server) { time(&Date); Code[0] = '\0'; } }; struct ServerState { -- cgit v1.2.3-18-g5258 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(-) 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-18-g5258 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(-) 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-18-g5258 From f4c4ffbacff336c98470123924f81032de4141cc Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 4 Feb 2021 00:44:53 +0100 Subject: Remove spurious periods on progress strings in po/de.po MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They result in what looks odd: Abhängigkeitsbaum wird aufgebaut.… Fertig Statusinformationen werden eingelesen.… Fertig --- po/de.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/de.po b/po/de.po index ee18e57f0..a66cdcdf7 100644 --- a/po/de.po +++ b/po/de.po @@ -1293,7 +1293,7 @@ msgstr "" #: apt-pkg/depcache.cc msgid "Building dependency tree" -msgstr "Abhängigkeitsbaum wird aufgebaut." +msgstr "Abhängigkeitsbaum wird aufgebaut" #: apt-pkg/depcache.cc msgid "Candidate versions" @@ -1305,7 +1305,7 @@ msgstr "Abhängigkeitsgenerierung" #: apt-pkg/depcache.cc msgid "Reading state information" -msgstr "Statusinformationen werden eingelesen." +msgstr "Statusinformationen werden eingelesen" #: apt-pkg/depcache.cc #, c-format -- cgit v1.2.3-18-g5258 From 131d0e3a261076da715102cb79275988cac810d1 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 4 Feb 2021 09:38:01 +0100 Subject: Prevent temporary directory from triggering failure grepping The grep for case-insensitive GPG finds also e.g. "/tmp/tmp.Kc5kKgPg0D" which is not the intention, so we simply eliminate the variation of the /tmp directory here from the output to prevent these false positives. Gbp-Dch: Ignore --- test/integration/test-bug-733028-gpg-resource-limit | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/test-bug-733028-gpg-resource-limit b/test/integration/test-bug-733028-gpg-resource-limit index 69baf4e5c..b44facda3 100755 --- a/test/integration/test-bug-733028-gpg-resource-limit +++ b/test/integration/test-bug-733028-gpg-resource-limit @@ -17,6 +17,7 @@ testaptkeys 'Joe Sixpack' testsuccess aptget update msgtest 'Test for no gpg errors/warnings in' 'apt-get update' +sed -i -e "s#${TMPWORKINGDIRECTORY}#/tmp/tmp.XXXXXX#g" rootdir/tmp/testsuccess.output if grep -iq 'GPG' rootdir/tmp/testsuccess.output; then cat rootdir/tmp/testsuccess.output msgfail -- cgit v1.2.3-18-g5258