Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics

[Aleksei Nikiforov <darktemplar@altlinux.org>]

07.12.2019 17:52, Andrey Savchenko пишет:
> Hi all!
> 
> On Fri,  6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote:
>> This change should fix pointer arithmetic issues for e2k.
> 
> This commit message is misleading. Pointer arithmetic is broken on
> all architectures, but by pure chance in happens to work correctly
> when gcc is used. On e2k the lcc compiler is used, so problem
> manifests itself. But this doesn't change the fact that pointer
> arithmetic was flawed on all architectures.
> 

Your first sentence contradicts later ones. You just repeated my 
sentence about issue manifesting on e2k, but you did it more verbosely. 
Yes, there is a flaw for all architectures, but it manifests only for 
e2k in practice, thus change fixes it for e2k. So, what's misleading there?

>> ---
>>   apt/apt-pkg/Makefile.am           |  1 +
>>   apt/apt-pkg/cacheiterators.h      | 26 ++++++++++++++------------
>>   apt/apt-pkg/contrib/mmap.cc       |  8 +++++---
>>   apt/apt-pkg/pkgcachegen.cc        | 29 +++++++++++++++--------------
>>   apt/apt-pkg/pkgcachegen.h         |  6 +++---
>>   apt/apt-pkg/rebase_pointer.h      | 16 ++++++++++++++++
>>   apt/apt-pkg/rpm/rpmlistparser.cc  |  5 +++--
>>   apt/apt-pkg/rpm/rpmpackagedata.cc |  2 +-
>>   8 files changed, 58 insertions(+), 35 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 a4bf670..118838e 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 <apt-pkg/rebase_pointer.h>
>> +
>>   #include <set>
>>   
>>   // Package Iterator
>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator
>>      inline unsigned long long Index() const {return Pkg - Owner->PkgP;};
>>      OkState State() const;
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap)
>> +   void ReMap(void *oldMap, void *newMap)
>>      {
>>         if (Owner == 0 || Pkg == 0)
>>            return;
>> -      Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
>> +      Pkg = RebasePointer(Pkg, oldMap, newMap);
>>      }
>>   
>>      // Constructors
>> @@ -147,11 +149,11 @@ class pkgCache::VerIterator
>>      bool Automatic() const;
>>      VerFileIterator NewestFile() const;
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap)
>> +   void ReMap(void *oldMap, void *newMap)
>>      {
>>         if (Owner == 0 || Ver == 0)
>>            return;
>> -      Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
>> +      Ver = RebasePointer(Ver, oldMap, newMap);
>>      }
>>   
>>      inline VerIterator() : Ver(0), Owner(0) {};
>> @@ -220,11 +222,11 @@ class pkgCache::DepIterator
>>      inline const char *CompType() {return Owner->CompType(Dep->CompareOp);};
>>      inline const char *DepType() {return Owner->DepType(Dep->Type);};
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap)
>> +   void ReMap(void *oldMap, void *newMap)
>>      {
>>         if (Owner == 0 || Dep == 0)
>>            return;
>> -      Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
>> +      Dep = RebasePointer(Dep, oldMap, newMap);
>>      }
>>   
>>      inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
>> @@ -279,11 +281,11 @@ class pkgCache::PrvIterator
>>      inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[Prv->Version].ParentPkg);};
>>      inline unsigned long long Index() const {return Prv - Owner->ProvideP;};
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap)
>> +   void ReMap(void *oldMap, void *newMap)
>>      {
>>         if (Owner == 0 || Prv == 0)
>>            return;
>> -      Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
>> +      Prv = RebasePointer(Prv, oldMap, newMap);
>>      }
>>   
>>      inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0)  {};
>> @@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator
>>      bool IsOk();
>>      string RelStr();
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap)
>> +   void ReMap(void *oldMap, void *newMap)
>>      {
>>         if (Owner == 0 || File == 0)
>>            return;
>> -      File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
>> +      File = RebasePointer(File, oldMap, newMap);
>>      }
>>   
>>      // Constructors
>> @@ -383,11 +385,11 @@ class pkgCache::VerFileIterator
>>      inline PkgFileIterator File() const {return PkgFileIterator(*Owner,FileP->File + Owner->PkgFileP);};
>>      inline unsigned long long Index() const {return FileP - Owner->VerFileP;};
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap)
>> +   void ReMap(void *oldMap, void *newMap)
>>      {
>>         if (Owner == 0 || FileP == 0)
>>            return;
>> -      FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
>> +      FileP = 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 cf01be9..35f1a25 100644
>> --- a/apt/apt-pkg/contrib/mmap.cc
>> +++ b/apt/apt-pkg/contrib/mmap.cc
>> @@ -30,6 +30,7 @@
>>   #include <apt-pkg/configuration.h>
>>   #include <apt-pkg/mmap.h>
>>   #include <apt-pkg/error.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>   
>>   #include <apti18n.h>
>>   
>> @@ -301,7 +302,7 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>>         Pool* oldPools = Pools;
>>         auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
>>         if (Pools != oldPools)
>> -         I += Pools - oldPools;
>> +         I = RebasePointer(I, oldPools, Pools);
>>   
>>         // Does the allocation failed ?
>>         if (!idxResult)
>> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size)
>>         Fd->Write(&C,sizeof(C));
>>      }
>>   
>> -   unsigned long const poolOffset = Pools - ((Pool*) Base);
>> +   void *old_base = Base;
>>   
>>      if (Fd != 0)
>>      {
>> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size)
>>         memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
>>      }
>>   
>> -   Pools = (Pool*) Base + poolOffset;
>> +   if (Base != old_base)
>> +      Pools = RebasePointer(Pools, old_base, Base);
>>      WorkSpace = newSize;
>>   
>>      return true;
>> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
>> index 654c81c..37364e9 100644
>> --- a/apt/apt-pkg/pkgcachegen.cc
>> +++ b/apt/apt-pkg/pkgcachegen.cc
>> @@ -26,6 +26,7 @@
>>   #include <apt-pkg/strutl.h>
>>   #include <apt-pkg/sptr.h>
>>   #include <apt-pkg/pkgsystem.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>   
>>   #include <apti18n.h>
>>   
>> @@ -109,18 +110,18 @@ pkgCacheGenerator::~pkgCacheGenerator()
>>      Map.Sync(0,sizeof(pkgCache::Header));
>>   }
>>   									/*}}}*/
>> -void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newMap)
>> +void pkgCacheGenerator::ReMap(void *oldMap, void *newMap)
>>   {
>>      if (oldMap == newMap)
>>         return;
>>   
>>      Cache.ReMap(false);
>>   
>> -   CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
>> +   CurrentFile = 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;
>> +         UniqHash[i] = RebasePointer(UniqHash[i], oldMap, newMap);
>>   
>>      for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
>>           i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
>> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>>   // CacheGenerator::WriteStringInMap					/*{{{*/
>>   std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String,
>>   					unsigned long Len) {
>> -   void const * const oldMap = Map.Data();
>> +   void *oldMap = Map.Data();
>>      const auto index = Map.WriteString(String, Len);
>>      if (index)
>>         ReMap(oldMap, Map.Data());
>> @@ -156,7 +157,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
>>   									/*}}}*/
>>   // CacheGenerator::WriteStringInMap					/*{{{*/
>>   std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(const char *String) {
>> -   void const * const oldMap = Map.Data();
>> +   void *oldMap = Map.Data();
>>      const auto index = Map.WriteString(String);
>>      if (index)
>>         ReMap(oldMap, Map.Data());
>> @@ -164,7 +165,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteStringInMap(cons
>>   }
>>   									/*}}}*/
>>   std::experimental::optional<map_ptrloc> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
>> -   void const * const oldMap = Map.Data();
>> +   void *oldMap = Map.Data();
>>      const auto index = Map.Allocate(size);
>>      if (index)
>>         ReMap(oldMap, Map.Data());
>> @@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>         pkgCache::VerIterator Ver = Pkg.VersionList();
>>         Dynamic<pkgCache::VerIterator> DynVer(Ver);
>>         map_ptrloc *Last = &Pkg->VersionList;
>> -      const void * oldMap = Map.Data();
>> +      void *oldMap = Map.Data();
>>         int Res = 1;
>>         for (; Ver.end() == false; Last = &Ver->NextVer, Ver++)
>>         {
>> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>   
>>         if (oldMap != Map.Data())
>>         {
>> -         Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> +         Last = RebasePointer(Last, oldMap, Map.Data());
>>            oldMap = Map.Data();
>>         }
>>   
>> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>>                                 PackageName.c_str(), 1);
>>   
>>         if (oldMap != Map.Data())
>> -         Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> +         Last = RebasePointer(Last, oldMap, Map.Data());
>>         *Last = *verindex;
>>   
>>         Ver->ParentPkg = Pkg.Index();
>> @@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>>   					       unsigned int Op,
>>   					       unsigned int Type)
>>   {
>> -   void const * const oldMap = Owner->Map.Data();
>> +   void *oldMap = Owner->Map.Data();
>>      pkgCache &Cache = Owner->Cache;
>>      
>>      // Get a structure
>> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>>   	 OldDepLast = &D->NextDepends;
>>         OldDepVer = Ver;
>>      } else if (oldMap != Owner->Map.Data())
>> -      OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
>> +      OldDepLast = RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
>>   
>>      // Is it a file dependency?
>>      if (PackageName[0] == '/')
>> @@ -739,7 +740,7 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>>      }
>>      
>>      // Get a structure
>> -   void const * const oldMap = Map.Data();
>> +   void *oldMap = Map.Data();
>>      const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
>>      const auto idxString = WriteStringInMap(S, Size);
>>      if ((!Item) || (!idxString))
>> @@ -747,8 +748,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>>   
>>      if (oldMap != Map.Data())
>>      {
>> -      Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
>> -      I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
>> +      Last = RebasePointer(Last, oldMap, Map.Data());
>> +      I = RebasePointer(I, oldMap, Map.Data());
>>      }
>>   
>>      *Last = *Item;
>> diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
>> index 77075a6..0faf6e1 100644
>> --- a/apt/apt-pkg/pkgcachegen.h
>> +++ b/apt/apt-pkg/pkgcachegen.h
>> @@ -72,7 +72,7 @@ class pkgCacheGenerator
>>      class DynamicFunction
>>      {
>>      public:
>> -      typedef std::function<void(const void *, const void *)> function;
>> +      typedef std::function<void(void *, void *)> function;
>>   
>>         static std::set<DynamicFunction*> toReMap;
>>   
>> @@ -93,7 +93,7 @@ class pkgCacheGenerator
>>            toReMap.erase(this);
>>         }
>>   
>> -      void call(const void *oldMap, const void *newMap)
>> +      void call(void *oldMap, void *newMap)
>>         {
>>            if (m_function)
>>               m_function(oldMap, newMap);
>> @@ -140,7 +140,7 @@ class pkgCacheGenerator
>>      // CNC:2003-03-18
>>      inline void ResetFileDeps() {FoundFileDeps = false;};
>>   
>> -   void ReMap(void const * const oldMap, void const * const newMap);
>> +   void ReMap(void *oldMap, void *newMap);
>>   
>>      pkgCacheGenerator(DynamicMMap *Map,OpProgress *Progress);
>>      ~pkgCacheGenerator();
>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
>> new file mode 100644
>> index 0000000..f6b3c15
>> --- /dev/null
>> +++ b/apt/apt-pkg/rebase_pointer.h
>> @@ -0,0 +1,16 @@
>> +#ifndef PKGLIB_REBASE_POINTER_H
>> +#define PKGLIB_REBASE_POINTER_H
>> +
>> +template <typename T>
>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base)
>> +{
>> +   return reinterpret_cast<T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +template <typename T>
>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base)
>> +{
>> +   return reinterpret_cast<const T*>(reinterpret_cast<char*>(new_base) + (reinterpret_cast<const char*>(ptr) - reinterpret_cast<char*>(old_base)));
>> +}
>> +
>> +#endif
>> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
>> index 9b2e9ad..bd13b99 100644
>> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
>> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
>> @@ -25,6 +25,7 @@
>>   #include <apt-pkg/strutl.h>
>>   #include <apt-pkg/crc-16.h>
>>   #include <apt-pkg/tagfile.h>
>> +#include <apt-pkg/rebase_pointer.h>
>>   
>>   #include <apti18n.h>
>>   
>> @@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>>      {
>>         SeenPackages.reset(new SeenPackagesType);
>>         m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunction(
>> -         [this] (void const *oldMap, void const *newMap)
>> +         [this] (void *oldMap, void *newMap)
>>         {
>>            SeenPackagesType tmp;
>>   
>>            for (auto iter: *SeenPackages)
>>            {
>> -            tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
>> +            tmp.insert(RebasePointer(iter, oldMap, newMap));
>>            }
>>   
>>            SeenPackages->swap(tmp);
>> diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpackagedata.cc
>> index 61c15d5..d6c677b 100644
>> --- a/apt/apt-pkg/rpm/rpmpackagedata.cc
>> +++ b/apt/apt-pkg/rpm/rpmpackagedata.cc
>> @@ -23,7 +23,7 @@ RPMPackageData::RPMPackageData()
>>   #endif
>>   {
>>      m_VerMapRealloc.reset(new pkgCacheGenerator::DynamicFunction(
>> -      [this] (void const *oldMap, void const *newMap)
>> +      [this] (void *oldMap, void *newMap)
>>      {
>>         for (auto iter1 = VerMap.begin(); iter1 != VerMap.end(); ++iter1)
>>         {
> 
> 
> Best regards,
> Andrew Savchenko
> 
> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>