Re: Static code-checking observations

[Gavin Smith]

On 4 February 2017 at 17:42, Hans-Bernhard Bröker <address@hidden> wrote:
>> Index: tilde.c
>> ===================================================================
>> --- tilde.c     (Revision 7649)
>> +++ tilde.c     (Arbeitskopie)
>> @@ -114,16 +114,13 @@
>>  char *
>>  tilde_expand (char *string)
>>  {
>> -  char *result;
>> -  int result_size, result_index;
>> +  char *result = NULL;
>> +  unsigned int result_size = 0, result_index = 0;
>>
>> -  result_size = result_index = 0;
>> -  result = NULL;
>> -
>>    /* Scan through STRING expanding tildes as we come to them. */
>>    while (1)
>>      {
>> -      register int start, end;
>> +      unsigned int start, end;
>>        char *tilde_word, *expansion;
>>        int len;
>>
>> @@ -132,7 +129,10 @@
>>
>>        /* Copy the skipped text into the result. */
>>        if ((result_index + start + 1) > result_size)
>> -        result = xrealloc (result, 1 + (result_size += (start + 20)));
>> +        {
>> +          result_size += start + 20;
>> +          result = xrealloc (result, 1 + result_size);
>> +        }
>>
>>        strncpy (result + result_index, string, start);
>>        result_index += start;
>>
>> I don't know what this is supposed to fix. The line in the log
>>
>> Logic error     Unix API
>>  info/tilde.c                            tilde_expand
>>   137     3
>>
>> refers to the line with "strncpy" on it.
>
>
> Exactly, and the trouble is that the checker sees a possiblity that
> strncpy's argument might be NULL.
>
> The trouble here is that `result' is NULL until the branch doing that
> xrealloc has taken place.  The changes to unsigned are (failed) attempts to
> allow the checker to see through  those calculations more easily and realize
> that the condition will always be true on the first pass, and therefore the
> xrealloc would always make result non-NULL.
>
> The ultimate obstacle may be xrealloc() here, though.  Its purpose is to
> never return unless it managed to make its output non-NULL.  But it's hard
> for the checker to figure that out cross-module.

tilde_expand isn't actually called anywhere, so I've removed it. :-)