[patch] various bugs and strncpy abuse followup

[Rob Landley]

Dropped:

ash.c: line 3955: bcmd = find_builtin(), then both of the goto builtin_success 
(3959 and 3979) test for bcmd not being null, so the only way we can get to 
the code you're adding a null check to is if cmdlookup() can't fail because 
it's a builtin command.

awk.c: What makes you think that op->r.n could be null but op->r.f in the very 
next case doesn't need this heck?  I'm not familiar enough with this code to 
know if this is a "can never happen" you want to check for, or if it's valid.  
I don't know.  But since it hasn't hit us yet, you don't have a test case 
that would trigger it, picking one case but not the other seems entirely 
arbitrary, I'm not going to spend any more time on it without further 
explanation.  (Try emailing awk's maintainer, Dmitry at the top of awk.c, to 
see what he says.  If he thinks it's needed, by all means resubmit...)

dpkg_deb.c: 1) Is dying because you couldn't create a subdirectory the correct 
behavior here?  (Printing an error message, yes, but should the code stop on 
the first error or continue?)  I dunno, ask Glenn McGrath.  2) This knocks 
out Makefile.in.patch, bb_mkdir.patch, and libbb.h.patch.  Since those 
patches all needed to be applied as a unit or not at all, it would be nice to 
have them in one patch file.

chown.c: This actually breaks the functionality.  If groupname is null (which 
the lines right above your change already check for) then --groupname should 
never equal *argv, because FFFFFFFF isn't going to be something argv points 
to, so we _want_ this logic to trigger when groupname is null.  As for the 
possibility that argv really does point to FFFFFFFF: not only is that a 
looney value (way up up past the kernel mappings) but in fact in Linux it's 
explicitly reserved for something else.  There's a page at the very top of 
memory the kernel reserves to catch references to decremented null pointers, 
which is why the sysenter page is one _before_ that.  Even if there was some 
strange hypothetical OS out there counting down from the top, argv[0] would 
have to come first and that can't be zero-length...

sed.c: line 766: "if(!(pattern_space=next_line)) break;" so if it was null we 
wouldn't have made it this far.  (The comment above that line is "advance to 
next line, stop if out of lines".)

loop.c: This breaks the code again.  Notice how rc had to be nonzero for us to 
get into this if() statement, and remains nonzero unless we successfully 
associate with a loop device, in which case we zero it (the line above your 
change).  That particular ioctl you want to instrument is failure recovery, 
if our attempt to grab the device fails, put it back how we found it.  If we 
_can't_ put it back how we found it, we don't particularly care, it might be 
the same problem that prevented us from grabbing it in the first place (such 
as the driver not being loaded) and all we care about is "couldn't grab it".  
The purpose of this function is either to associate one loop device (in which 
case we want rc to indicate success or failure, and your change overwrites rc 
with the result of the CLR_FD ioctl so if we couldn't grab it but 
successfully put it back how we found it, then we think we've grabbed it and 
report success), or to find and grab the first free loop device (in which 
case the above rc thing is still a problem, but in addition we don't want to 
return for a failure to associate when a later loop device may succeed, so 
inserting a return here at all is wrong because we want to continue).

I did, however, just notice a race condition here because of this, and fixed 
it: http://busybox.net/downloads/patches/svn-15509.patch

More in a bit...
-- 
Never bet against the cheap plastic solution.