Fishy code in sticky emulation?

Fishy code in sticky emulation?

[Mikael Magnusson]

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?

-- 
Mikael Magnusson

Re: Fishy code in sticky emulation?

[Peter Stephenson]

On Mon, 5 Jan 2015 15:34:00 +0100
Mikael Magnusson <mikachu@gmail.com> 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

Re: Fishy code in sticky emulation?

[Mikael Magnusson]

On Mon, Jan 5, 2015 at 3:48 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Mon, 5 Jan 2015 15:34:00 +0100
> Mikael Magnusson <mikachu@gmail.com> 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.

Ah, oops. I guess the getdata() functions are too involved for
coverity to follow (it gets confused by way easier things than that
too), and for me too :). Thanks, I'll mark it as a false positive.

-- 
Mikael Magnusson