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

["René Scharfe" <l.s.r@web.de>]

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.

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):


diff --git a/cache.h b/cache.h
index 595582becc..fe10789959 100644
--- a/cache.h
+++ b/cache.h
@@ -707,7 +707,7 @@ int daemonize(void);
  */
 #define ALLOC_GROW(x, nr, alloc) \
 	do { \
-		if ((nr) > alloc) { \
+		if (!alloc || (nr) > alloc) { \
 			if (alloc_nr(alloc) < (nr)) \
 				alloc = (nr); \
 			else \