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/pkgcachegen.cc | 63 ++++++++++++++++++-------------------------------- 1 file changed, 23 insertions(+), 40 deletions(-) (limited to 'apt-pkg/pkgcachegen.cc') 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; -- 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/pkgcachegen.cc') 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