Re: [RFC] cocci: .buf in a strbuf object can never be NULL

From: Jeff King <peff@peff.net>
Date: Mar 20, 2026, 04:18 UTC
Message-ID: <20260320041803.GA18125@coredump.intra.peff.net>
In-Reply-To: <xmqqcy0zgtmu.fsf@gitster.g>

On Thu, Mar 19, 2026 at 06:46:33PM -0700, Junio C Hamano wrote:

Show 25 quoted lines

> > diff --git a/strbuf.c b/strbuf.c
> > index 3939863cf3..0333aea261 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)
> >  #ifdef HAVE_GETDELIM
> >  int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
> >  {
> > +	char *buf;
> > +	size_t alloc;
> >  	ssize_t r;
> >  
> >  	if (feof(fp))
> > @@ -639,12 +641,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
> >  	strbuf_reset(sb);
> >  
> >  	/* Translate slopbuf to NULL, as we cannot call realloc on it */
> > -	if (!sb->alloc)
> > -		sb->buf = NULL;
> > +	alloc = sb->alloc;
> > +	buf = alloc ? sb->buf : NULL;
> >  	errno = 0;
> 
> I actually was hoping that all lines in this hunk before this point
> can be removed, i.e., strbuf_release(sb), buf = NULL, alloc = 0.

I'm not quite sure what you mean. The function right now looks like:

          ssize_t r;
  
          if (feof(fp))
                  return EOF;
  
          strbuf_reset(sb);
  
          /* Translate slopbuf to NULL, as we cannot call realloc on it */
          if (!sb->alloc)
                  sb->buf = NULL;
          errno = 0;

I think the strbuf_reset() could go away even without any other changes. We always adjust sb->len in the end to match what happened with getdelim(), so there is no point in doing it up front.

We could strbuf_release() and set buf to NULL, but that would defeat the purpose of the function, wouldn't it? We want to reuse sb->buf in each call, not allocate it fresh each time. I.e., in a loop like:

  while (strbuf_getline(&sb) != EOF) {
     ...look at sb.buf...
  }

we want to use the same buffer over and over.

Show 19 quoted lines

> > @@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
> >  	 * we can just re-init, but otherwise we should make sure that our
> >  	 * length is empty, and that the result is NUL-terminated.
> >  	 */
> > -	if (!sb->buf)
> > +	if (!buf)
> >  		strbuf_init(sb, 0);
> > -	else
> > -		strbuf_reset(sb);
> > +	else {
> > +		sb->buf = buf;
> > +		sb->alloc = alloc;
> > +		strbuf_reset(&sb);
> > +	}
> 
> I do not get all these conditionals.  This is an EOF code path; we
> have no data in buf to return.  We resetted the caller's strbuf
> already.  Can't we return buf (if allocated) to the system and
> return without doing any further damage to sb at this point?

The conditional is trying to keep any allocated buffer returned from getdelim() attached to the strbuf. Since this is EOF (or error), I agree it would probably be OK to just free it. Even in a loop like the one above, the loop will generally end at EOF, and we don't care about reusing the buffer further.

But it's not quite enough to just do:

  free(buf);

Because "buf" is a copy of sb->buf, and we handed "buf" off to getdelim(), we need to make sure sb->buf is not still pointing there. I think it would be enough to do:

  free(buf);
  strbuf_init(sb, 0);

If we did a strbuf_release() at the top of the function then that is not a concern (you know that sb->buf is pointing at the slopbuf). But I don't think that is a good idea for the reason I gave above.

-Peff