Re: Static analyzers results on musl

[Rich Felker <dalias@aerifal.cx>]

On Fri, Oct 04, 2013 at 09:51:25PM +0400, Alexander Monakov wrote:
>  - 2 sizeof mismatch warnings make sense

These have been fixed (as well as at least one other that was not
caught).

>  - 19+1 "dead code" warnings are helpful

I think some of these have been fixed, but they are low priority.

>  - "Out-of-bound array access" in glob.c appears to be a false positive (?)

At first I thought this was possibly a real overflow, but it seems to
be caused by the invalid use of [1] instead of [] for a flexible array
member in struct match. That's a bug in itself, so I'll look into
fixing it, but need to be careful not to mess up the allocation size
logic at the same time.

>  - There are many "garbage"/"undefined" warnings where the variable in
>    question is passed to a syscall by reference and expected to be initialized
>    there, unless error is signalled; it's quite unfortunate to have many false
>    positives like that

At least one of these seems to be a valid error:

http://port70.net/~nsz/musl/clang-2013-10-04/report-c1ebd3.html#EndPath

Unless the compiler takes advantage of the fact that accessing
indeterminate values is not valid, this one should have zero impact,
but needs to be fixed. I think reordering the operands of && would fix
it.

This one is also semi-valid:

http://port70.net/~nsz/musl/clang-2013-10-04/report-edc7bc.html#EndPath

But the code path it's taken is where the application has provided an
invalid stack address for the new thread, such that after aligning it
mod 16 and subtracting off __pthread_tsd_size, the resulting address
is null. However, valid pointer arithmetic can never result in a null
pointer, so I think this is actually a bug in clang's static analysis.

Please let me know if this analysis seems wrong.

>  - I have not attempted to investigate "dereference of null" warnings

The ones in regex are pretty complex and I'm still unclear on whether
the code paths flagged by the analysis are actually possible. It
doesn't help that this is third-party code. As for wordexp, I need to
look again; it looked to me like the null pointer dereference path
might occur when there are errors communicating with the child
process.

> I also have results from another static analysis tool developed internally
> were I work.  Here's a few hand-picked additional warnings.  I ran the tool
> without updating git first, so the tree was from September 9 (commit ff4be70).
> Sorry about that.
> 
> setenv.c:21  malloc return value not checked

Fixed.

> vfprintf.c:664
> vfwprint.c:354  va_end not called on error return path

Fixed.

> regcomp.c:767
> regcomp.c:807  sizeof mismatch; don't know why not flagged by clang

Fixed.

> getifaddrs.c:92  the code trusts the kernel that the fifth token would not be
> longer than IFNAMSIZ :)

Still pending whether we should consider this case.

Rich