[Toybox] [Patches] - Some more static analysis fixes

[Rob Landley]

On 10/07/14 00:50, Ashwini Sharma wrote:
> Hi Rob, List,
> 
> Attached are few more static analysis fixes. This includes issues like
> resource leaks, logical dead codes etc...
> 
> chgrp.c: Issue is in case when CFG_FREE and verbose mode enabled.
> Garbage output was there.
> 
> comm.c: loop error

You can tell I almost never test with TOYBOX_FREE enabled. (Even nommu 
doesn't need it.)

> id.c: single build issue for OLD_TOYs, same issue is there with some
> other, e.g. __mv__ 

The problem is each function can only be in one flag context (the 
FLAG_X macros are FOR_commandname) and if the command whose flag 
context a chunk of code is in is disabled, all the flag macros are 0 so 
dead code elimination can drop out.

So if you build mv without cp enabled, the fact mv's flags are 
intentionally the same as cp's doesn't help: the FLAG_ values in the 
function are zero.

I've made it about halfway through the patch list here, some comments:

--- cp.c.patch:

Why this change?

-    if (CFG_CP_MV && toys.which->name[0] == 'm') rc = rename(src, TT.destname);
+    if (CFG_CP_MV && toys.which->name[0] == 'm' && (toys.optflags & FLAG_f))
+      rc = rename(src, TT.destname);

$ strace mv umount umount.old 2>&1 | grep rename
rename("umount", "umount.old")          = 0

The hosts's umount is using mv without -f...?

(The other thing is a clear bug and shows that our cp.test needs a test
to spot that.)

--- fdisk.c.patch:

Given that name only gets used if the sscanf returns 4 (in which case it
copied a string over it, ignoring any previous contents), why clear it at all?

--- getty.c.patch:

I actually have a pending cleanup to getty I haven't checked in for ages
because I haven't got a serial port setup lying around to test. It redoes
the speed calculatons to look like:

// Find speed from mapper array
static speed_t encode(char *s)
{
  long speed = atolx(s);
  unsigned speeds[] = {50, 75, 110, 134, 150, 200, 300, 600, 1200, 1800,
    2400, 4800, 9600, 19200, 38400, 57600, 115200, 230400, 460800, 500000,
    576000, 721600, 1000000, 1152000, 1500000, 2000000, 2500000, 3000000,
    4000000}, i;

  for (i = 0; i<ARRAY_LEN(speeds); i++)
    if (speed == speeds[i]) return i + (i>14 ? 4096-14 : 1);

  return 0;
}

Which is _probably_ subtly wrong, and certainly not the kind of thing I'd
check in without testing. (I need to swing by Fry's and pick up a couple
USB->serial converters, the ones I had were packed in a box after the flood
and I wouldn't know where to start looking for them...)

I added the close(fd) to my copy, but can't easily check it in yet...

--- killall.c.patch

What I was actually depending on was that names_to_pid() wouldn't return
a string longer than 256 because that's the VFS limit on filenames and
name_to_pid won't return a filename can't match, but you're right they can
also specify an absolute _path_ to a command...

Rob