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> From: Aleksei Nikiforov Message-ID: <5daca19e-e674-62be-0971-3f4e1e8be9c3@altlinux.org> Date: Mon, 9 Dec 2019 09:54:27 +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: <20191207175201.13ca3df6c2cfb01ef559b083@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: Mon, 09 Dec 2019 06:55:42 -0000 Archived-At: List-Archive: List-Post: 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 >> + >> #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 >