Re: Static analyzers results on musl

[Rich Felker <dalias@aerifal.cx>]

On Fri, Oct 04, 2013 at 09:51:25PM +0400, Alexander Monakov wrote:
> Hello,
> 
> >From reading recent archives, it appeared to me there was some interest in
> applying source code analysis tools to musl.  My co-workers helped me run a
> couple of tools on musl, so here are the results.
> 
> Szabolcs kindly assisted with hosting Clang Analyzer results at
> 
>   http://port70.net/~nsz/musl/clang-2013-10-04/  
> 
> The analyzer was run on today's sources (commit 38a0a4d).  The build with
> make -j4 was interrupted at some point during building PIC objects; I presume
> at that point all non-PIC code was built, and the analyzer saw all source
> code, except maybe some #ifdef SHARED sections.
> 
> My take on those:
>  - 2 sizeof mismatch warnings make sense

Indeed, I think these are bugs, but it's likely they don't matter
because the allocations are larger than needed rather than smaller.

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

Yes.

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

I'll need to look closer at this. It might be a real issue.

>  - 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
>  - I have not attempted to investigate "dereference of null" warnings

Some of these look like they might be valid errors.

> 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

Definitely a bug. Fixing it.

> getspnam_r.c  I wonder if there's a window between opening the file and
> pthread_cleanup_push where the handle would leak? (this is not what the tool
> flagged)

No, there are no calls to cancellation points in that interval.

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

There are several cases of this in other places too. It has no
practical consequence, since the only possible implementation of
va_end is a no-op, but it should be fixed to make the code formally
correct.

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

Presumably because it's using a custom allocation function clang does
not know about.

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

This is an interesting theoretical issue we should probably adopt a
policy on. Obviously you have to trust the kernel to _some_ extent,
but there may be instances where it makes sense to validate data from
the kernel.

> There are a few warnings that return value of .*nl_langinfo.* is not checked
> for NULL before use; presumably that is by design.

nl_langinfo is not permitted to return NULL, so this warning makes no
sense.

Rich