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

[Christian Grothoff]

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.

Happy hacking!

Christian