Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics

["Dmitry V. Levin" <ldv@altlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote:
> 09.12.2019 2:21, Dmitry V. Levin пишет:
> > On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote:
> > [...]
> >> @@ -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)
> > 
> > Is there any particular reason for stripping const here and in other
> > similar places?
> 
> Yes, it's needed due to issues emerging from mixing const and non-const 
> pointers with new and allegedly more proper way of calculating rebased 
> pointers.

Sorry, I don't find this argument convincing.
I have experienced no const issues in my version of this fix.

> > [...]
> >> @@ -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)
> > 
> > In my patch RebasePointer invocation was after the idxResult check,
> > not before the check.
> 
> Theoretically, order here might be important. In practice, it doesn't 
> matter.

We normally try to write code that raises less questions.

> > By the way, in this and other similar cases,
> > is there any reason for "Pools != oldPools" check?
> > Is RebasePointer incapable of handling this, or is it an optimization?
> > 
> 
> It's just an optimization, it may be removed.

OK

> > [...]
> >> 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
> > 
> > Do we really need two templates here?
> 
> Yes, second template with const ptr is needed for 
> rpmListParser::rpmListParser from rpmlistparser.cc.
> 
> Variable SeenPackages has type SeenPackagesType, which is a typedef to 
> std::set<const char*,cstr_lt_pred>. Thus, elements are 'const char*', 
> and either it should be const-casted to 'char*', which is ugly, or 
> const-correctness should be achieved some other way, for example by 
> getting rid of unimportant const qualifiers like in my changes.
> 
> And first template is needed for every other case with non-const ptr.

To be honest, I find my October version of the fix easier to read.

Since all users of RebasePointer except rpmListParser use it in a form of
	ptr = RebasePointer(ptr, old_base, new_base);
I find it more natural when RebasePointer updates the pointer,
so one can write
	RebasePointer(ptr, old_base, new_base);
instead.

OK, I posted my version of the fix.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]