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: <20191206133647.dculnmwkd3yf2wjp@titan.localdomain> <20191206153655.86334-1-darktemplar@altlinux.org> <20191207175201.13ca3df6c2cfb01ef559b083@altlinux.org> <5daca19e-e674-62be-0971-3f4e1e8be9c3@altlinux.org> <20191212222006.fe1de5270a370f446e085241@altlinux.org> From: Aleksei Nikiforov Message-ID: Date: Fri, 13 Dec 2019 10:58:26 +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: <20191212222006.fe1de5270a370f446e085241@altlinux.org> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit Subject: Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics 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: Fri, 13 Dec 2019 07:58:32 -0000 Archived-At: List-Archive: List-Post: 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 >>>> + >>>> #include >>>> >>>> // 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(newMap) - static_cast(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(newMap) - static_cast(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(newMap) - static_cast(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(newMap) - static_cast(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(newMap) - static_cast(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(newMap) - static_cast(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 >>>> #include >>>> #include >>>> +#include >>>> >>>> #include >>>> >>>> @@ -301,7 +302,7 @@ std::experimental::optional 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 >>>> #include >>>> #include >>>> +#include >>>> >>>> #include >>>> >>>> @@ -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::toReMap.begin(); >>>> i != Dynamic::toReMap.end(); ++i) >>>> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM >>>> // CacheGenerator::WriteStringInMap /*{{{*/ >>>> std::experimental::optional 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 pkgCacheGenerator::WriteStringInMap(cons >>>> /*}}}*/ >>>> // CacheGenerator::WriteStringInMap /*{{{*/ >>>> std::experimental::optional 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 pkgCacheGenerator::WriteStringInMap(cons >>>> } >>>> /*}}}*/ >>>> std::experimental::optional 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 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 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 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 function; >>>> + typedef std::function function; >>>> >>>> static std::set 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 >>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_base) >>>> +{ >>>> + return reinterpret_cast(reinterpret_cast(new_base) + (reinterpret_cast(ptr) - reinterpret_cast(old_base))); >>>> +} >>>> + >>>> +template >>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, void *new_base) >>>> +{ >>>> + return reinterpret_cast(reinterpret_cast(new_base) + (reinterpret_cast(ptr) - reinterpret_cast(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 >>>> #include >>>> #include >>>> +#include >>>> >>>> #include >>>> >>>> @@ -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(newMap) - static_cast(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 >