From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 To: devel@lists.altlinux.org References: <20191209235406.GA15810@altlinux.org> <20191209235654.GC15867@altlinux.org> From: Aleksei Nikiforov Message-ID: Date: Tue, 10 Dec 2019 11:18:24 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20191209235654.GC15867@altlinux.org> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit Subject: Re: [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: Tue, 10 Dec 2019 08:18:52 -0000 Archived-At: List-Archive: List-Post: 10.12.2019 2:56, Dmitry V. Levin пишет: > 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 This line is not true. I didn't participate in creation of this version of patch. Please remove it. > 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); >