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@xxxxxxxxxxxxx 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@xxxxxxxxx> 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@xxxxxxxxx>
---
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
