Static analyzers results on musl

Static analyzers results on musl

[Alexander Monakov]

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
 - 19+1 "dead code" warnings are helpful
 - "Out-of-bound array access" in glob.c appears to be a false positive (?)
 - 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


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

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)

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

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

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

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

Hope that helps.

Alexander

Re: Static analyzers results on musl

[Szabolcs Nagy]

* Alexander Monakov <amonakov@ispras.ru> [2013-10-04 21:51:25 +0400]:
> >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.

thanks for doing this

> Szabolcs kindly assisted with hosting Clang Analyzer results at
> 
>   http://port70.net/~nsz/musl/clang-2013-10-04/  

temporarily available

i fixed two minor math issues, commits are in my review branch

Re: Static analyzers results on musl

[Rich Felker]

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

Re: Static analyzers results on musl

[Alexander Monakov]

[apologies if you receive this email twice]

On Fri, 4 Oct 2013, Rich Felker wrote:
> > 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.

I meant when internal functions like __nl_langinfo_l are called -- this one
can return NULL.  Sorry for causing confusion with ".*".

Alexander

Re: Static analyzers results on musl

[Rich Felker]

On Sat, Oct 05, 2013 at 01:10:24AM +0400, Alexander Monakov wrote:
> [apologies if you receive this email twice]
> 
> On Fri, 4 Oct 2013, Rich Felker wrote:
> > > 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.
> 
> I meant when internal functions like __nl_langinfo_l are called -- this one
> can return NULL.  Sorry for causing confusion with ".*".

But there's no reason to expect that it could return NULL unless this
is documented as a possibility. Does the tool you're using assume that
any function which returns a pointer might return NULL?

Rich

Re: Static analyzers results on musl

[Alexander Monakov]

On Fri, 4 Oct 2013, Rich Felker wrote:
> But there's no reason to expect that it could return NULL unless this
> is documented as a possibility. Does the tool you're using assume that
> any function which returns a pointer might return NULL?

In src/locale/langinfo.c, __nl_langinfo_l _explicitely_ returns NULL in 5
distinct locations, as of this time.  Am I misunderstanding something?

Alexander

Re: Static analyzers results on musl

[Rich Felker]

On Sat, Oct 05, 2013 at 01:39:46AM +0400, Alexander Monakov wrote:
> On Fri, 4 Oct 2013, Rich Felker wrote:
> > But there's no reason to expect that it could return NULL unless this
> > is documented as a possibility. Does the tool you're using assume that
> > any function which returns a pointer might return NULL?
> 
> In src/locale/langinfo.c, __nl_langinfo_l _explicitely_ returns NULL in 5
> distinct locations, as of this time.  Am I misunderstanding something?

Indeed, I hadn't read the code lately. But those are all places where
the behavior is undefined due to invalid input. They should probably
be removed.

Rich

Re: Static analyzers results on musl

[Rich Felker]

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