From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 10 Dec 2019 02:56:56 +0300 From: "Dmitry V. Levin" To: ALT Devel discussion list Message-ID: <20191209235654.GC15867@altlinux.org> References: <20191209235406.GA15810@altlinux.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191209235406.GA15810@altlinux.org> Subject: [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Dec 2019 23:56:57 -0000 Archived-At: List-Archive: List-Post: Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9 among other changes introduced UB in pointer arithmetic by casting raw pointers to specific types. Fix this by introducing two helpers for rebasing pointers in a safe way. Co-developed-by: Aleksei Nikiforov Fixes: 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes") --- apt/apt-pkg/Makefile.am | 1 + apt/apt-pkg/cacheiterators.h | 14 ++++++++------ apt/apt-pkg/contrib/mmap.cc | 8 ++++---- apt/apt-pkg/pkgcachegen.cc | 27 +++++++++++---------------- apt/apt-pkg/rebase_pointer.h | 25 +++++++++++++++++++++++++ apt/apt-pkg/rpm/rpmlistparser.cc | 3 ++- 6 files changed, 51 insertions(+), 27 deletions(-) create mode 100644 apt/apt-pkg/rebase_pointer.h diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am index 4c0d234..d038d01 100644 --- a/apt/apt-pkg/Makefile.am +++ b/apt/apt-pkg/Makefile.am @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \ pkgsystem.h \ policy.cc \ policy.h \ + rebase_pointer.h \ repository.cc \ repository.h \ scopeexit.h \ diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h index 9dffeb3..3c60cb8 100644 --- a/apt/apt-pkg/cacheiterators.h +++ b/apt/apt-pkg/cacheiterators.h @@ -34,6 +34,8 @@ #pragma interface "apt-pkg/cacheiterators.h" #endif +#include + // Package Iterator class pkgCache::PkgIterator { @@ -87,7 +89,7 @@ class pkgCache::PkgIterator { if (Owner == 0 || Pkg == 0) return; - Pkg += static_cast(newMap) - static_cast(oldMap); + RebasePointer(Pkg, oldMap, newMap); } // Constructors @@ -149,7 +151,7 @@ class pkgCache::VerIterator { if (Owner == 0 || Ver == 0) return; - Ver += static_cast(newMap) - static_cast(oldMap); + RebasePointer(Ver, oldMap, newMap); } inline VerIterator() : Ver(0), Owner(0) {}; @@ -222,7 +224,7 @@ class pkgCache::DepIterator { if (Owner == 0 || Dep == 0) return; - Dep += static_cast(newMap) - static_cast(oldMap); + RebasePointer(Dep, oldMap, newMap); } inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) : @@ -281,7 +283,7 @@ class pkgCache::PrvIterator { if (Owner == 0 || Prv == 0) return; - Prv += static_cast(newMap) - static_cast(oldMap); + RebasePointer(Prv, oldMap, newMap); } inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {}; @@ -344,7 +346,7 @@ class pkgCache::PkgFileIterator { if (Owner == 0 || File == 0) return; - File += static_cast(newMap) - static_cast(oldMap); + RebasePointer(File, oldMap, newMap); } // Constructors @@ -385,7 +387,7 @@ class pkgCache::VerFileIterator { if (Owner == 0 || FileP == 0) return; - FileP += static_cast(newMap) - static_cast(oldMap); + RebasePointer(FileP, oldMap, newMap); } inline VerFileIterator() : Owner(0), FileP(0) {}; diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc index 2064fc4..779d7a6 100644 --- a/apt/apt-pkg/contrib/mmap.cc +++ b/apt/apt-pkg/contrib/mmap.cc @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -285,13 +286,12 @@ std::experimental::optional DynamicMMap::Allocate(unsigned long Item I->Count = size/ItemSize; Pool* oldPools = Pools; auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize); - if (Pools != oldPools) - I += Pools - oldPools; // Does the allocation failed ? if (!idxResult) return idxResult; + RebasePointer(I, oldPools, Pools); Result = *idxResult; I->Start = Result; } @@ -356,7 +356,7 @@ bool DynamicMMap::Grow(unsigned long long size) Fd->Write(&C,sizeof(C)); } - unsigned long const poolOffset = Pools - ((Pool*) Base); + const void *old_base = Base; if (Fd != 0) { @@ -393,7 +393,7 @@ bool DynamicMMap::Grow(unsigned long long size) memset((char*)Base + WorkSpace, 0, newSize - WorkSpace); } - Pools = (Pool*) Base + poolOffset; + RebasePointer(Pools, old_base, Base); WorkSpace = newSize; return true; diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 56716b5..7a5a20c 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include @@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM Cache.ReMap(false); - CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap; + RebasePointer(CurrentFile, oldMap, newMap); for (size_t i = 0; i < _count(UniqHash); ++i) if (UniqHash[i] != 0) - UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap; + RebasePointer(UniqHash[i], oldMap, newMap); for (auto i = Dynamic::toReMap.begin(); i != Dynamic::toReMap.end(); ++i) @@ -269,11 +270,8 @@ bool pkgCacheGenerator::MergeList(ListParser &List, continue; } - if (oldMap != Map.Data()) - { - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap; - oldMap = Map.Data(); - } + RebasePointer(Last, oldMap, Map.Data()); + oldMap = Map.Data(); // Skip to the end of the same version set. if (Res == 0) @@ -296,8 +294,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List, return _error->Error(_("Error occurred while processing %s (NewVersion%d)"), PackageName.c_str(), 1); - if (oldMap != Map.Data()) - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap; + RebasePointer(Last, oldMap, Map.Data()); *Last = *verindex; Ver->ParentPkg = Pkg.Index(); @@ -604,8 +601,9 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver, for (pkgCache::DepIterator D = Ver.DependsList(); D.end() == false; D++) OldDepLast = &D->NextDepends; OldDepVer = Ver; - } else if (oldMap != Owner->Map.Data()) - OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap; + } else { + RebasePointer(OldDepLast, oldMap, Owner->Map.Data()); + } // Is it a file dependency? if (PackageName[0] == '/') @@ -745,11 +743,8 @@ std::experimental::optional pkgCacheGenerator::WriteUniqString(const if ((!Item) || (!idxString)) return std::experimental::optional(); - if (oldMap != Map.Data()) - { - Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap; - I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap; - } + RebasePointer(Last, oldMap, Map.Data()); + RebasePointer(I, oldMap, Map.Data()); *Last = *Item; diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h new file mode 100644 index 0000000..2bbabea --- /dev/null +++ b/apt/apt-pkg/rebase_pointer.h @@ -0,0 +1,25 @@ +#ifndef PKGLIB_REBASE_POINTER_H +#define PKGLIB_REBASE_POINTER_H + +template +static inline T* +GetRebasedPointer(T*, const void *, const void *) +__attribute__((__warn_unused_result__)); + +template +static inline T* +GetRebasedPointer(T* ptr, const void *old_base, const void *new_base) +{ + // uintptr_t is a type with well-defined integer overflow semantics + uintptr_t diff = (uintptr_t) new_base - (uintptr_t) old_base; + return (T*) ((uintptr_t) ptr + diff); +} + +template +static inline void +RebasePointer(T* &ptr, const void *old_base, const void *new_base) +{ + ptr = GetRebasedPointer(ptr, old_base, new_base); +} + +#endif diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc index 9b2e9ad..4aeb937 100644 --- a/apt/apt-pkg/rpm/rpmlistparser.cc +++ b/apt/apt-pkg/rpm/rpmlistparser.cc @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler) for (auto iter: *SeenPackages) { - tmp.insert(iter + (static_cast(newMap) - static_cast(oldMap))); + tmp.insert(GetRebasedPointer(iter, oldMap, newMap)); } SeenPackages->swap(tmp); -- ldv