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

[Andrey Savchenko <bircoph@altlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 18080 bytes --]

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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]