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-70-g09d2