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

[Aleksei Nikiforov <darktemplar@altlinux.org>]

12.12.2019 22:20, Andrey Savchenko пишет:
> On Mon, 9 Dec 2019 09:54:27 +0300 Aleksei Nikiforov wrote:
>> 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.
> 
> No.
> 
>> You just repeated my sentence about issue manifesting on e2k,
> 
> No.
> 
>> but you did it more verbosely.
> 
> Yes.
> 
>> 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?
> 
> When a commit message is of concern it does matter what is written,
> and what was intended but kept behind the scenes is of no
> importance.
> 
> When one writes "issue A is fixed for B" this implies that A is
> B-specific issue, which is false in the discussed case.
> 

Is it a complain to latest patch version or old one? If it's to old one, 
I'll just skip it.

>>>> ---
>>>>    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
>>>
>> _______________________________________________
>> Devel mailing list
>> Devel@lists.altlinux.org
>> https://lists.altlinux.org/mailman/listinfo/devel
> 
> Best regards,
> Andrew Savchenko
> 
> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>