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

Aleksei Nikiforov darktemplar на altlinux.org
Ср Дек 11 10:50:22 MSK 2019

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

Подробная информация о списке рассылки Devel