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 Date: Thu, 12 Dec 2019 22:20:06 +0300 From: Andrey Savchenko To: ALT Linux Team development discussions Message-Id: <20191212222006.fe1de5270a370f446e085241@altlinux.org> In-Reply-To: <5daca19e-e674-62be-0971-3f4e1e8be9c3@altlinux.org> References: <20191206133647.dculnmwkd3yf2wjp@titan.localdomain> <20191206153655.86334-1-darktemplar@altlinux.org> <20191207175201.13ca3df6c2cfb01ef559b083@altlinux.org> <5daca19e-e674-62be-0971-3f4e1e8be9c3@altlinux.org> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; i686-pc-linux-gnu) Mime-Version: 1.0 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg="PGP-SHA512"; boundary="Signature=_Thu__12_Dec_2019_22_20_06_+0300_aXrojaQ9IdWUzyWR" 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: Thu, 12 Dec 2019 19:20:16 -0000 Archived-At: List-Archive: List-Post: --Signature=_Thu__12_Dec_2019_22_20_06_+0300_aXrojaQ9IdWUzyWR Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, 9 Dec 2019 09:54:27 +0300 Aleksei Nikiforov wrote: > 07.12.2019 17:52, Andrey Savchenko =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > Hi all! > >=20 > > On Fri, 6 Dec 2019 18:36:55 +0300 Aleksei Nikiforov wrote: > >> This change should fix pointer arithmetic issues for e2k. > >=20 > > 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. > >=20 >=20 > 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=20 > e2k in practice, thus change fixes it for e2k. So, what's misleading ther= e? 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 =3D \ > >> 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 > >> =20 > >> +#include > >> + > >> #include > >> =20 > >> // Package Iterator > >> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator > >> inline unsigned long long Index() const {return Pkg - Owner->PkgP= ;}; > >> OkState State() const; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >> { > >> if (Owner =3D=3D 0 || Pkg =3D=3D 0) > >> return; > >> - Pkg +=3D static_cast(newMap) - static_cast(oldMap); > >> + Pkg =3D RebasePointer(Pkg, oldMap, newMap); > >> } > >> =20 > >> // Constructors > >> @@ -147,11 +149,11 @@ class pkgCache::VerIterator > >> bool Automatic() const; > >> VerFileIterator NewestFile() const; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >> { > >> if (Owner =3D=3D 0 || Ver =3D=3D 0) > >> return; > >> - Ver +=3D static_cast(newMap) - static_cast(oldMap); > >> + Ver =3D RebasePointer(Ver, oldMap, newMap); > >> } > >> =20 > >> inline VerIterator() : Ver(0), Owner(0) {}; > >> @@ -220,11 +222,11 @@ class pkgCache::DepIterator > >> inline const char *CompType() {return Owner->CompType(Dep->Compar= eOp);}; > >> inline const char *DepType() {return Owner->DepType(Dep->Type);}; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >> { > >> if (Owner =3D=3D 0 || Dep =3D=3D 0) > >> return; > >> - Dep +=3D static_cast(newMap) - static_cast<= Dependency const *>(oldMap); > >> + Dep =3D RebasePointer(Dep, oldMap, newMap); > >> } > >> =20 > >> inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * =3D = 0) : > >> @@ -279,11 +281,11 @@ class pkgCache::PrvIterator > >> inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Ow= ner->PkgP + Owner->VerP[Prv->Version].ParentPkg);}; > >> inline unsigned long long Index() const {return Prv - Owner->Prov= ideP;}; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >> { > >> if (Owner =3D=3D 0 || Prv =3D=3D 0) > >> return; > >> - Prv +=3D static_cast(newMap) - static_cast(oldMap); > >> + Prv =3D RebasePointer(Prv, oldMap, newMap); > >> } > >> =20 > >> inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0) {}; > >> @@ -342,11 +344,11 @@ class pkgCache::PkgFileIterator > >> bool IsOk(); > >> string RelStr(); > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >> { > >> if (Owner =3D=3D 0 || File =3D=3D 0) > >> return; > >> - File +=3D static_cast(newMap) - static_cas= t(oldMap); > >> + File =3D RebasePointer(File, oldMap, newMap); > >> } > >> =20 > >> // Constructors > >> @@ -383,11 +385,11 @@ class pkgCache::VerFileIterator > >> inline PkgFileIterator File() const {return PkgFileIterator(*Owne= r,FileP->File + Owner->PkgFileP);}; > >> inline unsigned long long Index() const {return FileP - Owner->Ve= rFileP;}; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >> { > >> if (Owner =3D=3D 0 || FileP =3D=3D 0) > >> return; > >> - FileP +=3D static_cast(newMap) - static_cast(oldMap); > >> + FileP =3D RebasePointer(FileP, oldMap, newMap); > >> } > >> =20 > >> 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 > >> =20 > >> #include > >> =20 > >> @@ -301,7 +302,7 @@ std::experimental::optional DynamicMMa= p::Allocate(unsigned long Item > >> Pool* oldPools =3D Pools; > >> auto idxResult =3D RawAllocate(I->Count*ItemSize,ItemSize); > >> if (Pools !=3D oldPools) > >> - I +=3D Pools - oldPools; > >> + I =3D RebasePointer(I, oldPools, Pools); > >> =20 > >> // Does the allocation failed ? > >> if (!idxResult) > >> @@ -371,7 +372,7 @@ bool DynamicMMap::Grow(unsigned long long size) > >> Fd->Write(&C,sizeof(C)); > >> } > >> =20 > >> - unsigned long const poolOffset =3D Pools - ((Pool*) Base); > >> + void *old_base =3D Base; > >> =20 > >> if (Fd !=3D 0) > >> { > >> @@ -408,7 +409,8 @@ bool DynamicMMap::Grow(unsigned long long size) > >> memset((char*)Base + WorkSpace, 0, newSize - WorkSpace); > >> } > >> =20 > >> - Pools =3D (Pool*) Base + poolOffset; > >> + if (Base !=3D old_base) > >> + Pools =3D RebasePointer(Pools, old_base, Base); > >> WorkSpace =3D newSize; > >> =20 > >> 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 > >> =20 > >> #include > >> =20 > >> @@ -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 =3D=3D newMap) > >> return; > >> =20 > >> Cache.ReMap(false); > >> =20 > >> - CurrentFile +=3D (pkgCache::PackageFile*) newMap - (pkgCache::Pack= ageFile*) oldMap; > >> + CurrentFile =3D RebasePointer(CurrentFile, oldMap, newMap); > >> =20 > >> for (size_t i =3D 0; i < _count(UniqHash); ++i) > >> if (UniqHash[i] !=3D 0) > >> - UniqHash[i] +=3D (pkgCache::StringItem*) newMap - (pkgCache:= :StringItem*) oldMap; > >> + UniqHash[i] =3D RebasePointer(UniqHash[i], oldMap, newMap); > >> =20 > >> for (auto i =3D Dynamic::toReMap.begin(); > >> i !=3D Dynamic::toReMap.end(); ++i) > >> @@ -147,7 +148,7 @@ void pkgCacheGenerator::ReMap(void const * const o= ldMap, void const * const newM > >> // CacheGenerator::WriteStringInMap /*{{{*/ > >> std::experimental::optional pkgCacheGenerator::WriteStri= ngInMap(const char *String, > >> unsigned long Len) { > >> - void const * const oldMap =3D Map.Data(); > >> + void *oldMap =3D Map.Data(); > >> const auto index =3D Map.WriteString(String, Len); > >> if (index) > >> ReMap(oldMap, Map.Data()); > >> @@ -156,7 +157,7 @@ std::experimental::optional pkgCacheGe= nerator::WriteStringInMap(cons > >> /*}}}*/ > >> // CacheGenerator::WriteStringInMap /*{{{*/ > >> std::experimental::optional pkgCacheGenerator::WriteStri= ngInMap(const char *String) { > >> - void const * const oldMap =3D Map.Data(); > >> + void *oldMap =3D Map.Data(); > >> const auto index =3D Map.WriteString(String); > >> if (index) > >> ReMap(oldMap, Map.Data()); > >> @@ -164,7 +165,7 @@ std::experimental::optional pkgCacheGe= nerator::WriteStringInMap(cons > >> } > >> /*}}}*/ > >> std::experimental::optional pkgCacheGenerator::AllocateI= nMap(unsigned long size) {/*{{{*/ > >> - void const * const oldMap =3D Map.Data(); > >> + void *oldMap =3D Map.Data(); > >> const auto index =3D Map.Allocate(size); > >> if (index) > >> ReMap(oldMap, Map.Data()); > >> @@ -229,7 +230,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List, > >> pkgCache::VerIterator Ver =3D Pkg.VersionList(); > >> Dynamic DynVer(Ver); > >> map_ptrloc *Last =3D &Pkg->VersionList; > >> - const void * oldMap =3D Map.Data(); > >> + void *oldMap =3D Map.Data(); > >> int Res =3D 1; > >> for (; Ver.end() =3D=3D false; Last =3D &Ver->NextVer, Ver++) > >> { > >> @@ -271,7 +272,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List, > >> =20 > >> if (oldMap !=3D Map.Data()) > >> { > >> - Last +=3D (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap; > >> + Last =3D RebasePointer(Last, oldMap, Map.Data()); > >> oldMap =3D Map.Data(); > >> } > >> =20 > >> @@ -297,7 +298,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List, > >> PackageName.c_str(), 1); > >> =20 > >> if (oldMap !=3D Map.Data()) > >> - Last +=3D (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap; > >> + Last =3D RebasePointer(Last, oldMap, Map.Data()); > >> *Last =3D *verindex; > >> =20 > >> Ver->ParentPkg =3D Pkg.Index(); > >> @@ -553,7 +554,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkg= Cache::VerIterator &Ver, > >> unsigned int Op, > >> unsigned int Type) > >> { > >> - void const * const oldMap =3D Owner->Map.Data(); > >> + void *oldMap =3D Owner->Map.Data(); > >> pkgCache &Cache =3D Owner->Cache; > >> =20 > >> // Get a structure > >> @@ -605,7 +606,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkg= Cache::VerIterator &Ver, > >> OldDepLast =3D &D->NextDepends; > >> OldDepVer =3D Ver; > >> } else if (oldMap !=3D Owner->Map.Data()) > >> - OldDepLast +=3D (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*)= oldMap; > >> + OldDepLast =3D RebasePointer(OldDepLast, oldMap, Owner->Map.Dat= a()); > >> =20 > >> // Is it a file dependency? > >> if (PackageName[0] =3D=3D '/') > >> @@ -739,7 +740,7 @@ std::experimental::optional pkgCacheGe= nerator::WriteUniqString(const > >> } > >> =20 > >> // Get a structure > >> - void const * const oldMap =3D Map.Data(); > >> + void *oldMap =3D Map.Data(); > >> const auto Item =3D AllocateInMap(sizeof(pkgCache::StringItem)); > >> const auto idxString =3D WriteStringInMap(S, Size); > >> if ((!Item) || (!idxString)) > >> @@ -747,8 +748,8 @@ std::experimental::optional pkgCacheGe= nerator::WriteUniqString(const > >> =20 > >> if (oldMap !=3D Map.Data()) > >> { > >> - Last +=3D (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap; > >> - I +=3D (pkgCache::StringItem*) Map.Data() - (pkgCache::StringIt= em*) oldMap; > >> + Last =3D RebasePointer(Last, oldMap, Map.Data()); > >> + I =3D RebasePointer(I, oldMap, Map.Data()); > >> } > >> =20 > >> *Last =3D *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 functio= n; > >> + typedef std::function function; > >> =20 > >> static std::set toReMap; > >> =20 > >> @@ -93,7 +93,7 @@ class pkgCacheGenerator > >> toReMap.erase(this); > >> } > >> =20 > >> - 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 =3D false;}; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap); > >> + void ReMap(void *oldMap, void *newMap); > >> =20 > >> 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) + (r= einterpret_cast(ptr) - reinterpret_cast(old_base))); > >> +} > >> + > >> +template > >> +static inline const T* RebasePointer(const T *ptr, void *old_base, vo= id *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/rpmlis= tparser.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 > >> =20 > >> #include > >> =20 > >> @@ -50,13 +51,13 @@ rpmListParser::rpmListParser(RPMHandler *Handler) > >> { > >> SeenPackages.reset(new SeenPackagesType); > >> m_SeenPackagesRealloc.reset(new pkgCacheGenerator::DynamicFunc= tion( > >> - [this] (void const *oldMap, void const *newMap) > >> + [this] (void *oldMap, void *newMap) > >> { > >> SeenPackagesType tmp; > >> =20 > >> for (auto iter: *SeenPackages) > >> { > >> - tmp.insert(iter + (static_cast(newMap) - st= atic_cast(oldMap))); > >> + tmp.insert(RebasePointer(iter, oldMap, newMap)); > >> } > >> =20 > >> SeenPackages->swap(tmp); > >> diff --git a/apt/apt-pkg/rpm/rpmpackagedata.cc b/apt/apt-pkg/rpm/rpmpa= ckagedata.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 =3D VerMap.begin(); iter1 !=3D VerMap.end(); += +iter1) > >> { > >=20 > >=20 > > Best regards, > > Andrew Savchenko > >=20 > >=20 > > _______________________________________________ > > Devel mailing list > > Devel@lists.altlinux.org > > https://lists.altlinux.org/mailman/listinfo/devel > >=20 > _______________________________________________ > Devel mailing list > Devel@lists.altlinux.org > https://lists.altlinux.org/mailman/listinfo/devel Best regards, Andrew Savchenko --Signature=_Thu__12_Dec_2019_22_20_06_+0300_aXrojaQ9IdWUzyWR Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE63ZIHsdeM+1XgNer9lNaM7oe5I0FAl3ykuYACgkQ9lNaM7oe 5I1Mdw//UsFC4zoB+H9QW/bhVfQs7Z/J+nqe1RxAjNQEirTE79yDaoLTh+S+uNUO RUnquZuM3JCJqjWQM9Qd3Tcjc5KlUIx6BeqJhMbC/0GrtcuiKKjbwan3aCBlFzDt QLYUPBXzWGIzFXiCr1ipaxpLSeOFcRrCGzrVJLyC02r5ezIkupGkBbMh4yKk1QVi 228oBDPT2FV8XTtHh6fLccsxQ10SJgG0dr+T6N4SttJH/FHBx3s9OB09BhOepvPU HBheGiF3+tBfLMg+QWVENt6GKmulcutFlYXYasd5pivHxh8Cus6ei2YXN8gawAGK w3Tl1d/vudCCr8ci5RpDnxPJJeXUZrsNbSJlToMirjWgzmDbkTobXZrDTR5AmD+R GauqAR1PSz5oZJHfZwZY04DIOCLqelwOGn9kXEMyBno4vwBkpZYDzdwU+jPa1pcy PdmPQDyMmqT2EZV4kMT2ZEZUEICAi65mY/D5tq0zz6VW4yeOjC6og1sDW0sktlOn m8CUTLcMtkSNjaoZ5gRVx8LEEg37ASHjcLOewOjnHCK9TZqCAMKEjkpFimrXtp5Q //E6xt9IoemUEdzUo5vPfc6bDjOazGVJX2kFiRBj1ggRmxFPVLoHD5/1JLvfnEv0 eyj0+yToffZjAr+5UvCXvQtYJaYqhtbiyQAR+7W7jgrIyTOAPH8= =ytZH -----END PGP SIGNATURE----- --Signature=_Thu__12_Dec_2019_22_20_06_+0300_aXrojaQ9IdWUzyWR--