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

[Aleksei Nikiforov <darktemplar@altlinux.org>]

11.12.2019 1:20, Dmitry V. Levin пишет:
> 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?
> 

In good C++ code there is no place for const_cast. Maybe there are some 
exceptions to it, but they have to be justified. It doesn't matter that 
you are hiding it behind C-style cast.

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

Ok, considering this rule, looks good.

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

Not as wasteful as you speculated it is. And no, it is centralized and 
generic. It fixes all issues caused by remainders of division being 
non-zero in pointer math.

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

It's possible to add -Werror=warning-type compiler option to convert 
specific warning into error. You wouldn't miss an error generated by 
compiler, would you? :) Btw, while we're discussing compiler options, 
you can turn errors back into warnings with -Wno-error=warning-type 
compiler option when using -Werror.

>> As for other potential issues, they are very far-fetched and synthetic.
> 
> Well, I don't think so. :)
> 

You can fix that :)

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