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

[Aleksei Nikiforov <darktemplar@altlinux.org>]

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++.

>>> [...]
>>>> @@ -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.

>>> 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.
> 

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.

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