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

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

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

On Tue, Dec 10, 2019 at 01:58:17PM +0300, Aleksei Nikiforov wrote:
> 10.12.2019 13:20, Dmitry V. Levin пишет:
> > 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?
> 
> I didn't say that something prevents from using C constructs when 
> appropriate. I'm saying that these constructs are not appropriate here.

Why do you think they are not appropriate here?

> >>>>>> @@ -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.
> 
> So, do you have any reason why it should be changed?

One of the most basic coding rules says: the return value that needs
checking has to be checked prior to any meaningful use.

> >>>>> [...]
> >>>>>> 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++.
> 
> It's also very easy to miss one of places where such pointer 
> recalculation is required,

There must be a way to exclude this possibility.

> but you still want this solution instead of 
> generic and centralized memory alignment one.

The approach you mentioned is definitely wasteful,
but it's by no means generic or centralized.

> So much for uniformity of approaches and solutions.
> 
> As for forgetting assignment, your addition of attribute 'warn unused 
> result' in your version of patch fixes this potential issue.

Unfortunately, warn_unused_result attribute does not fix anything yet
because it's too easy to miss a new warning among several hundreds of
already existing warnings.  This might help someday in the future when
the whole codebase is ready for -Werror.

> As for other potential issues, they are very far-fetched and synthetic.

Well, I don't think so. :)


-- 
ldv

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