[devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
Aleksei Nikiforov
darktemplar на altlinux.org
Пт Дек 13 10:58:26 MSK 2019
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 <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
>
>
> _______________________________________________
> Devel mailing list
> Devel на lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>
Подробная информация о списке рассылки Devel