From: Andrey Savchenko <bircoph@altlinux.org>
To: ALT Linux Team development discussions <devel@lists.altlinux.org>
Subject: Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics
Date: Sat, 7 Dec 2019 17:52:01 +0300
Message-ID: <20191207175201.13ca3df6c2cfb01ef559b083@altlinux.org> (raw)
In-Reply-To: <20191206153655.86334-1-darktemplar@altlinux.org>
[-- Attachment #1: Type: text/plain, Size: 15582 bytes --]
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.
> ---
> 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
[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-12-07 14:52 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-06 13:16 [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Aleksei Nikiforov
2019-12-06 13:16 ` [devel] [PATCH for apt 2/2] Fix pointer arithmetics Aleksei Nikiforov
2019-12-06 13:36 ` Ivan A. Melnikov
2019-12-06 15:32 ` Aleksei Nikiforov
2019-12-06 15:36 ` [devel] [PATCH for apt 2/2 v2] " Aleksei Nikiforov
2019-12-07 14:52 ` Andrey Savchenko [this message]
2019-12-08 22:56 ` Dmitry V. Levin
2019-12-09 6:54 ` Aleksei Nikiforov
2019-12-12 19:20 ` Andrey Savchenko
2019-12-13 7:58 ` Aleksei Nikiforov
2019-12-08 23:21 ` Dmitry V. Levin
2019-12-09 7:08 ` Aleksei Nikiforov
2019-12-10 0:07 ` Dmitry V. Levin
2019-12-10 8:18 ` Aleksei Nikiforov
2019-12-10 10:20 ` Dmitry V. Levin
2019-12-10 10:58 ` Aleksei Nikiforov
2019-12-10 22:20 ` Dmitry V. Levin
2019-12-11 7:50 ` Aleksei Nikiforov
2019-12-12 19:43 ` Andrey Savchenko
2019-12-13 8:01 ` Aleksei Nikiforov
2019-12-08 23:31 ` [devel] [PATCH for apt 2/2] " Dmitry V. Levin
2019-12-09 7:09 ` Aleksei Nikiforov
2019-12-09 7:16 ` [devel] [PATCH for apt 2/2 v3] " Aleksei Nikiforov
2019-12-09 23:54 ` [devel] [PATCH apt 0/3] Fix 0.5.15lorg2-alt70~9 fallout Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 1/3] apt-pkg/cacheiterators.h: revert #include <set> Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 2/3] apt-pkg/contrib/mmap.cc: revert confusing change of Flags to this->Flags Dmitry V. Levin
2019-12-09 23:56 ` [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic Dmitry V. Levin
2019-12-10 8:18 ` Aleksei Nikiforov
2019-12-08 22:50 ` [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate Dmitry V. Levin
2019-12-09 6:58 ` Aleksei Nikiforov
2019-12-09 10:24 ` Dmitry V. Levin
2019-12-09 11:03 ` [devel] [PATCH for apt 1/2 v3] Add Debug::DynamicMMap::Allocate option Aleksei Nikiforov
2019-12-09 22:59 ` Dmitry V. Levin
2019-12-09 7:00 ` [devel] [PATCH for apt 1/2 v2] Add option for debugging Debug::DynamicMMap::Allocate Aleksei Nikiforov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191207175201.13ca3df6c2cfb01ef559b083@altlinux.org \
--to=bircoph@altlinux.org \
--cc=devel@lists.altlinux.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
ALT Linux Team development discussions
This inbox may be cloned and mirrored by anyone:
git clone --mirror http://lore.altlinux.org/devel/0 devel/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 devel devel/ http://lore.altlinux.org/devel \
devel@altlinux.org devel@altlinux.ru devel@lists.altlinux.org devel@lists.altlinux.ru devel@linux.iplabs.ru mandrake-russian@linuxteam.iplabs.ru sisyphus@linuxteam.iplabs.ru
public-inbox-index devel
Example config snippet for mirrors.
Newsgroup available over NNTP:
nntp://lore.altlinux.org/org.altlinux.lists.devel
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git