Re: Fishy code in sticky emulation?

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Mon, 5 Jan 2015 15:34:00 +0100
Mikael Magnusson <mikachu@xxxxxxxxx> wrote:
> I'm looking through Coverity issues (some patches to come later), and
> it flagged this in builtin.c that I can't quite say for sure if it's
> right or wrong about.
> 
> int
> bin_emulate(UNUSED(char *nam), char **argv, Options ops, UNUSED(int func))
> {
> ...
>     if (sticky->n_on_opts)
>       on_ptr = sticky->on_opts =
>         zhalloc(sticky->n_on_opts * sizeof(*sticky->on_opts));
>     else
>       on_ptr = NULL;
>     if (sticky->n_off_opts)
>       off_ptr = sticky->off_opts = zhalloc(sticky->n_off_opts *
>                                    sizeof(*sticky->off_opts));
>     else
>       off_ptr = NULL;
>     for (optnode = firstnode(optlist); optnode; incnode(optnode)) {
>       /* Data is index into new_opts */
>       char *optptr = (char *)getdata(optnode);
>       int optno = optptr - new_opts;
>       if (*optptr)
>         *on_ptr++ = optno;
>       else
>         *off_ptr++ = optno;
>       }
> ...
> 
> In particular, on_ptr and off_ptr can be NULL, but unconditionally one
> of them is always incremented in the for loop, which isn't very well
> defined for a NULL pointer. Am I missing something, or are these
> n_*_opts simply never 0?

You missed out the previous loop which is exactly parallel to the second
one you quoted:

    for (optnode = firstnode(optlist); optnode; incnode(optnode)) {
	/* Data is index into new_opts */
	char *optptr = (char *)getdata(optnode);
	if (*optptr)
	    sticky->n_on_opts++;
	else
	    sticky->n_off_opts++;
    }

This means that in the second loop on_ptr must be non-NULL whenever
*optptr is non-NULL and off_ptr must be non-NULL whenever it's NULL.

However, it would be fine to make the test for the pointers more
explicit; it's not going to make any noticeable difference even if you
run emulate very frequently.

pws