[devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
Andrey Savchenko
bircoph на altlinux.org
Чт Дек 12 22:20:06 MSK 2019
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
----------- следующая часть -----------
Было удалено вложение не в текстовом формате...
Имя : отсутствует
Тип : application/pgp-signature
Размер : 833 байтов
Описание: отсутствует
Url : <http://lists.altlinux.org/pipermail/devel/attachments/20191212/2aede95c/attachment-0001.bin>
Подробная информация о списке рассылки Devel