[Toybox] [Patches] - Static analysis fixes

[Rob Landley]

On 08/11/14 23:01, Ashwini Sharma wrote:
> Hi Andre,
> 
> I used _prevent_ by Coverity and also a proprietary tool.
> 
> regards,
> Ashwini 

Durign the time my email machine was dead (turned out to be bad ram) I
applied these fixes. I wrote up notes in a file and forgot to post them.

Static analysis:

The ifconfig.c and modprobe.c changes are good, applied.

I'm going to assume the cleanups to stuff still in pending are good and
apply them; the resulting code can be reviewed when the commands are.

Except... These patches need -p2 instead of -p1 to apply, and have no
newline on the last line so ubuntu's patch command complains with:

  patch unexpectedly ends in middle of line
  Hunk #3 succeeded at 301 with fuzz 1.

The arp.c patch is indenting with tabs instead of spaces in the last
hunk. So is the modprobe.c patch. (*shrug* Fixed...)

uniq.c is slightly more complicated:

This potentially closes the global stdin and stdout streams (that's what
they're initialized to in the no argument case). If the streams
underlying those structures live on the heap instead of a malloc (I
don't see c99 expressing an opinion either way, looks like it's up to
the libc implementation) then this free can segfault on the way out of
the command.

The CFG_TOYBOX_FREE stanzas are mostly there to reduce valgrind noise
from people who like testing that sort of thing. They also potentially
allow more commands to be called in nofork mode, which could be useful
if you glue toybox onto a bootloader that doesn't have a real VM so
can't cleanup after exiting processes. But the big problem with that is
the various error_exit() paths have a longjmp to intercept control flow
instead of exiting, but but no infrastructure to actually clean anything
up yet. (Allocations, filehandles, mmaps... There's a partial list of
process attributes in "man 2 execve" but that doesn't include
process-local attributes like the heap, or attributes that _are_
preserved across an exec. Something as simple as not resetting umask can
introduce subtle bugs...)

So if we call error_exit() via xopen() (or xmalloc(), xchdir()...),
then CFG_TOYBOX_FREE is just the tip of the iceberg. That's why I've
leaned on the OS doing cleanup for us, so far. Basically waiting for
nommu guys to show up and express interest in having the next layer of
infrastructure implemented.

That said, as long as we're doing this: We can avoid closing stdout
by comparing the pointers with the stdin/stdout globals.

>     I've done some investigation into these recently, and found them to be
>     quite nice to integrate into the build system, although the open
>     source ones (cppcheck, sparse and frama-c) don't seem to be nearly as
>     comprehensive as the commercial ones. I think it's something that
>     every C project should run at least periodically, if not on every
>     build.

I generally refer to them as false positive generators, but if somebody
wants to look at the output and spot potential actual fixes, I'm all for
it...

Rob

 1408457307.0