Re: [devel] [PATCH for apt v2 21/21] Fix invalid check of Queue against zero

[Aleksei Nikiforov <darktemplar@altlinux.org>]

12.12.2019 22:08, Andrey Savchenko пишет:
> On Thu, 12 Dec 2019 12:57:30 +0300 Aleksei Nikiforov wrote:
>> Queue must not be zero in this function, otherwise it'd crash in this function
>> anyway, since it's used like it's never zero later.
>> Found via clang-static-analyzer:
>> Logic error: Called C++ object pointer is null:
>> Called C++ object pointer is null
>> ---
>>   apt/apt-pkg/acquire-method.cc | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/apt/apt-pkg/acquire-method.cc b/apt/apt-pkg/acquire-method.cc
>> index 9a3ef1d..3b5c580 100644
>> --- a/apt/apt-pkg/acquire-method.cc
>> +++ b/apt/apt-pkg/acquire-method.cc
>> @@ -555,9 +555,7 @@ void pkgAcqMethod::Warning(const char *Format,...)
>>      to keep the pipeline synchronized. */
>>   void pkgAcqMethod::Redirect(const string &NewURI)
>>   {
>> -   string CurrentURI = "<UNKNOWN>";
>> -   if (Queue != 0)
>> -      CurrentURI = Queue->Uri;
>> +   string CurrentURI = Queue->Uri;
> 
> If (Queue == NULL) this code will result in the NULL pointer
> dereference. It does not follow from the code that Queue is never
> NULL.
> 
> So this may be a problem in the static analyzer. Even if it is not,
> such safeguard elimination is not safe for future code
> modifications.
> 
> BTW GCC is good at DCE (dead code elimination), so if this check is
> really useless, it will be omitted from the binary code.
> 

If you read full code of this function, you'll see that even if Queue is 
NULL, it's still dereferenced later, and thus this check is excessive. 
No, it doesn't mean that it's never NULL. It just gets rid of excessive 
check.

DCE is a good stuff, but it's much better to not keep it in source code 
if it's truly dead.

>>      ostringstream s;
>>      s << "103 Redirect\nURI: " << CurrentURI << "\nNew-URI: " << NewURI
> 
> 
> Best regards,
> Andrew Savchenko
> 
> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>