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

[Aleksei Nikiforov <darktemplar@altlinux.org>]

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.

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

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

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

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