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

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

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

On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote:
> 10.12.2019 3:07, Dmitry V. Levin пишет:
> > 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.
> 
> Your version is using C-style casts in C++ code. Of course, I could use 
> C-style casts or const_cast-s too to work around const correctness 
> issues (i.e. to just hide these issues), and it'd work like your 
> version. But I'd like to remind you that APT is C++ project, not a C 
> project. What might be ok for C is sometimes a dirty ugly hack in modern 
> C++.

Sorry, I don't share you point of view on this matter.
Being a C++ project allows you to use C++ constructs, that's true,
but why do you think it prevents you from using C constructs when
appropriate?

> >>>> @@ -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.
> 
> In that case it's better to keep order from my patch, isn't it? 
> Practically it's fine either way, but theoretically that order is superior.

The order in question was introduced by your commit
6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes").

If I was reviewing that commit, this would have been fixed already.

> >>> [...]
> >>>> 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.
> 
> And it's opposite for me. I prefer to explicitly see when variable is 
> changed. And for all calls it looks exactly same: no matter how it's 
> used, new pointer is returned from function as a result of function. 
> Interface uniformity, obviousness and predictability is important.

What I don't like in your approach is that it's error-prone:
it's very easy to forget the assignment or to assign the result to a wrong
variable.  In fact, I had to use the following regular expression just
to check whether all uses of RebasePointer are correct in that respect:

$ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\) = RebasePointer(\1,'

This is surely not the way how things should be done,
neither in C nor in C++.


-- 
ldv

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