Re: What's cooking in git.git (Oct 2022, #03; Mon, 10)

From: Jeff King <peff@peff.net>
Date: Dec 22, 2022, 17:40 UTC
Message-ID: <Y6SWfOQrvOkLM67P@coredump.intra.peff.net>
In-Reply-To: <221222.867cyjx0d3.gmgdl@evledraar.gmail.com>

On Thu, Dec 22, 2022 at 09:58:01AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 12 quoted lines

> > I do think it would be less noisy if we could somehow convince Coverity
> > that yes, strbuf really does NUL-terminate the result. But I haven't
> > wanted to sink time into figuring out how to annotate it.
> 
> I don't have Coverity set up, but perhaps it's satisfied by the same
> thing that placeted GCC's -fanalyzers in strbuf.c:
> 
> 	https://lore.kernel.org/git/RFC-patch-07.15-cf1a5f3ed0f-20220603T183608Z-avarab@gmail.com/
> 
> I run my local build with a version of that branch, I'd still like to
> follow-up on it (and as that RFC thread shows others had some alternate
> suggestions, e.g. for this strbuf case).

I don't think that will help. The most common strbuf problem in Coverity is "this string isn't NUL terminated". And having walked through their step-by-step analysis, I think what is going on is that it sees that:

  strbuf_addstr(&sb, "foo");

is doing:

  memcpy(sb->buf, "foo", strlen(foo));

under the hood, and it says "aha, this is an anti-pattern where you forgot to copy the NUL byte!" and creates a warning. And it ignores completely the fact that the next line is calling strbuf_setlen() and adding the NUL byte.

Now there may be other false positives around strbufs (like not realizing the buffer grows), but this is the one I feel like I've seen the most.

Show 8 quoted lines

> I don't think it's true that a strbuf "really does NUL-terminate the
> result" the way an analyzer like -fanalyzer sees it. I.e. if you do:
> 
> 	struct strbuf sb = { .alloc = 123 };
> 	strbuf_addstr(&sb, "blah");
> 
> You'll segfault because the sb->buf isn't the slopbuf, nor
> '\0'-terminated, it's just NULL.

Yeah, I didn't mean to say that there can't be real problems with strbufs. I just meant that there are many false positives where the code is correct, but the tool doesn't realize it.

-Peff