Re: [libmicrohttpd] Clang static analyzer reports...

[Tim Rühsen]

On 05.02.20 19:29, Christian Grothoff wrote:
> On Mon, 2020-02-03 at 16:09 +0100, Tim Rühsen wrote:
>> Hi Christian,
>> On 2/3/20 3:44 PM, Christian Grothoff wrote:
>>> Hi Tim,
>>>
>>> Thanks for forwarding the report. I've looked through them. The
>>> first
>>> two clang is pissy because we don't annotate with 'nonnull'.  Doing
>>> so
>>> *consistently* would be a lot of work, if someone wants to do so,
>>> great, but I won't for the near future.
>>
>> Not exactly, clang is pissy because you explicitly give NULL as
>> argument
>> to a non-null annotated function argument.
> 
> Actually, the invariant is clear: either digest is NULL, XOR password
> is NULL. To help static analyzers, we even already have an mhd_assert
> (NULL != password) which is actually relatively easy to derive as
> satisfied when looking at the two call sites.  So yes, we do explicitly
> give NULL to one of the arguments, but only if the other one is non-
> NULL. Anyway, I've added another assert to maybe help clang understand
> this in 2d4289dc..a2103adb. If it does, great, if not, tough luck.
> 
>>> The "logic error" (NULL dereference) looks very much like a logic
>>> error
>>> in CLANG. It boils down to:
>>>
>>>
>>> state = 42;
>>> ptr = NULL;
>>> switch (state)
>>> {
>>> case 42:
>>>   perfectly safe;
>>>   break;
>>> case 44:
>>>   deref ptr;
>>>   break;
>>> }
>>>
>>> and clang goes for the wrong case (44) even though 42 was just set
>>> 5
>>> statements above.  So yes, a logic error, but in clang ;-).
>>
>> The 'state = 42' is set somewhere outside the function with the
>> switch
>> (MHD_connection_handle_idle).
>>
>> From looking at the function, you (at least I) can't say if 'state'
>> is
>> always set to 42 before calling it. Even if this is the case right
>> now -
>> this sounds like a pitfall for any developer who is not 100% firm
>> with
>> the code.
>>
>> It might be a matter of favor to clean this up or not. An alternative
>> is
>> a clang analyzer suppression file. Keeping the status like it is
>> effectively disables automatic flaw detection by the CI - in this
>> case I
>> would simply disable/remove the runner.
> 
> Well, looking at the 50 step history, clang does not seem to reference
> a possible execution of that other file. Regardless, at the time where
> the idle-function sets the state to 42, it just basically a few lines
> above dereferenced the pointer that must not be NULL, so yeah, it's
> pretty assured to be non-NULL at the time.
> 
> I'm not against adding suppressions, as there might be other issues
> clang might catch in the future. So please send a patch to suppress
> these false positives.

As it turned out, the scan-build tool doesn't support suppression files.
And understandably, no project maintainer (including me) likes
tool-specific annotations in the code itself.

Did anyone already play with gcc-10's -fanalyzer ?

https://gcc.gnu.org/onlinedocs/gcc/Static-Analyzer-Options.html

Regards, Tim

signature.asc
Description: OpenPGP digital signature