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

[Aleksei Nikiforov <darktemplar@altlinux.org>]

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.

>>>>>> @@ -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?

>>>>> [...]
>>>>>> 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, but you still want this solution instead of 
generic and centralized memory alignment one. 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. As for 
other potential issues, they are very far-fetched and synthetic.

> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>