| Message ID | RFC-patch-07.15-cf1a5f3ed0f-20220603T183608Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode | expand |
Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: > 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!"); new_buf is !sb->alloc. ALLOC_GROW sets buf if sb->len + extra + 1 is bigger than sb->alloc, which is always true because the variables in the sum are unsigned, so we end up with at least 1 > 0. That's easy enough to see; I wonder why the compiler doesn't agree. Am I missing something? Does setting the attribute returns_nonnull for xrealloc help? Specifying it explicitly makes sense to me -- expecting the compiler to infer it automatically across compilation units is probably a bit too much to ask for, or at least needlessly expensive. > if (new_buf) > sb->buf[0] = '\0'; > }
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] > 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() ? Best Wishes Phillip > if (new_buf) > sb->buf[0] = '\0'; > } > -- > 2.36.1.1124.g577fa9c2ebd >
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.
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 \
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
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!"); if (new_buf) sb->buf[0] = '\0'; }
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(+)