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 _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
