Re: Fishy code in sticky emulation?

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Mon, Jan 5, 2015 at 3:48 PM, Peter Stephenson
<p.stephenson@xxxxxxxxxxx> wrote:
> 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.

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