Re: [RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()

[Phillip Wood <phillip.wood123@gmail.com>]

On 04/06/2022 21:37, René Scharfe wrote:
> Am 04.06.22 um 18:21 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Sat, Jun 04 2022, Phillip Wood wrote:
>>
>>> Hi Ævar
>>>
>>> [This is an old address that I only have webmail access to, please use
>>> phillip.wood@dunelm.org.uk when cc'ing me]
>>
>> Hrm, I just grabbed it from an old commit of yours I found. Consider
>> sending in a patch to update .mailmap :)
>>
>>>> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>>
>>>>
>>>> Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't
>>>> yell at us about sb->buf[0] dereferencing NULL, this also makes this
>>>> code easier to follow.
>>>>
>>>> This BUG() should be unreachable since the state of our "sb->buf" and
>>>> "sb->alloc" goes hand-in-hand, but -fanalyzer isn't smart enough to
>>>> know that, and adding the BUG() also makes it clearer to human readers
>>>> that that's what happens here.
>>>>
>>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>>> ---
>>>>   strbuf.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/strbuf.c b/strbuf.c
>>>> index dd9eb85527a..61c4630aeeb 100644
>>>> --- a/strbuf.c
>>>> +++ b/strbuf.c
>>>> @@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
>>>>   	if (new_buf)
>>>>   		sb->buf = NULL;
>>>>   	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
>>>> +	if (new_buf && !sb->buf)
>>>> +		BUG("for a new buffer ALLOC_GROW() should always do work!");
>>>
>>> This is a bit ugly, have you tried adding
>>> __attribute__((malloc (free), returns_nonnull))
>>> to xmalloc() and xrealloc() ?
>>>
>>
>> Will try to experiment with that, perhaps GCC can be massaged to grok
>> this somehow.
>>
>> I do vaguely remember (but couldn't track down where it was) that we
>> have some config for coverity for this function, due to it also having
>> trouble with it.
> 
> Good idea, but this attribute doesn't seem to help here.

Indeed, I was tricked by the misleading BUG message, the analyzer is not 
complaining that xrealloc() may return NULL, it is taking a path where 
it does not reallocate the buffer. I've copied the full output at the 
end of the message if anyone wants to see the whole thing but the 
relevant part is

            |cache.h:727:20:
            |  727 |                 if ((nr) > alloc) { \
            |      |                    ^
            |      |                    |
            |      |                    (10) following ‘false’ branch...
strbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’
            |   99 |         ALLOC_GROW(sb->buf, sb->len + extra + 1, 
sb->alloc);
            |      |         ^~~~~~~~~~

> The following helps, but I don't know why it would be needed -- if alloc
> is 0 then any strbuf_grow() call will give a nr of at least 1 (for the
> NUL character):

Yes it seems to be ignoring the result of our overflow checks and 
assuming that sb->len + extra + 1 can overflow to zero. Having seen the 
number of false positives I'm inclined to think we should take the bug 
fixes and punt the rest of these patches. Maybe after a couple more gcc 
releases the false positive rate will be more manageable. (I tired 
running gcc 11 with -fanalyzer a few months ago and gave up looking at 
the output before I found a single real bug so it has certainly improved 
in gcc 12)

Best Wishes

Phillip

strbuf.c: In function ‘strbuf_grow’:
strbuf.c:101:28: warning: dereference of NULL ‘0’ [CWE-476] 
[-Wanalyzer-null-dereference]
   101 |                 sb->buf[0] = '\0';
       |                 ~~~~~~~~~~~^~~~~~
   ‘strbuf_normalize_path’: events 1-2
     |
     | 1156 | int strbuf_normalize_path(struct strbuf *src)
     |      |     ^~~~~~~~~~~~~~~~~~~~~
     |      |     |
     |      |     (1) entry to ‘strbuf_normalize_path’
     |......
     | 1160 |         strbuf_grow(&dst, src->len);
     |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~
     |      |         |
     |      |         (2) calling ‘strbuf_grow’ from ‘strbuf_normalize_path’
     |
     +--> ‘strbuf_grow’: events 3-4
            |
            |   91 | void strbuf_grow(struct strbuf *sb, size_t extra)
            |      |      ^~~~~~~~~~~
            |      |      |
            |      |      (3) entry to ‘strbuf_grow’
            |......
            |   94 |         if (unsigned_add_overflows(extra, 1) ||
            |      |            ~
            |      |            |
            |      |            (4) following ‘false’ branch (when 
‘extra != 18446744073709551615’)...
            |
          ‘strbuf_grow’: event 5
            |
            |   95 |             unsigned_add_overflows(sb->len, extra + 1))
git-compat-util.h:128:7: note: in definition of macro 
‘unsigned_add_overflows’
            |  128 |     ((b) > maximum_unsigned_value_of_type(a) - (a))
            |      |       ^
            |
          ‘strbuf_grow’: events 6-9
            |
            |strbuf.c:94:46:
            |   94 |         if (unsigned_add_overflows(extra, 1) ||
            |......
            |   97 |         if (new_buf)
            |      |         ~~ ~
            |      |         |  |
            |      |         |  (8) following ‘true’ branch...
            |      |         (7) ...to here
            |   98 |                 sb->buf = NULL;
            |      |                 ~~
            |      |                 |
            |      |                 (9) ...to here
            |
          ‘strbuf_grow’: event 10
            |
            |cache.h:727:20:
            |  727 |                 if ((nr) > alloc) { \
            |      |                    ^
            |      |                    |
            |      |                    (10) following ‘false’ branch...
strbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’
            |   99 |         ALLOC_GROW(sb->buf, sb->len + extra + 1, 
sb->alloc);
            |      |         ^~~~~~~~~~
            |
          ‘strbuf_grow’: event 11
            |
            |cache.h:726:12:
            |  726 |         do { \
            |      |            ^
            |      |            |
            |      |            (11) ...to here
strbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’
            |   99 |         ALLOC_GROW(sb->buf, sb->len + extra + 1, 
sb->alloc);
            |      |         ^~~~~~~~~~
            |
          ‘strbuf_grow’: events 12-15
            |
            |  100 |         if (new_buf)
            |      |            ^
            |      |            |
            |      |            (12) following ‘true’ branch...
            |  101 |                 sb->buf[0] = '\0';
            |      |                 ~~~~~~~~~~~~~~~~~
            |      |                 | |        |
            |      |                 | |        (15) dereference of NULL 
‘*sb.buf’
            |      |                 | (14) ‘dst.buf’ is NULL
            |      |                 (13) ...to here