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

From

Jeff King <peff@peff.net>

Date

Mar 19, 2026, 23:35 UTC

Message-ID

<20260319233546.GA3632561@coredump.intra.peff.net>

In-Reply-To

<xmqqcy0zii0s.fsf@gitster.g>

On Thu, Mar 19, 2026 at 03:14:27PM -0700, Junio C Hamano wrote:

> One is in list-objects-filter-options.c::parse_list_objects_filter()
> 
>         void parse_list_objects_filter(
>                 struct list_objects_filter_options *filter_options,
>                 const char *arg)
>         {
>                 struct strbuf errbuf = STRBUF_INIT;
> 
>                 if (!filter_options->filter_spec.buf)
>                         BUG("filter_options not properly initialized");
> 
> The filter_options variable points at a list_objects_filter_options
> structure, which has an embedded "struct strbuf".  This BUG() is
> unnecessary if the structure is properly initialized, either by the
> LIST_OBJECTS_FILTER_INIT macro or a list_objects_filter_init() call.
> But it is easy to memset(&lofo, 0, sizeof(lofo)) or zero initialize
> with "= {0}", so I think it is OK to special case and allow for
> checking the possibility that .buf might be NULL.

> Because strbuf_getwholeline() discards what is originally in sb and
> replaces it with what getdelim() returns, I have a suspicion that
> working with bare char * and size_t to interact with getdelim() and
> then using strbuf_attach() on the success case would be simpler to
> read and maintain.  Once such a rewrite of this function is done
> (#leftoverbits), the special case we see in the Coccinelle rule can
> be lifted.

Hmm. I think that is something like this:

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;
-	r = getdelim(&sb->buf, &sb->alloc, term, fp);
+	r = getdelim(&buf, &alloc, term, fp);
 
 	if (r > 0) {
+		sb->buf = buf;
+		sb->alloc = alloc;
 		sb->len = r;
 		return 0;
 	}
@@ -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);
+	}
 	return EOF;
 }
 #else

So I don't know that it makes anything simpler. We have to copy the
values back into the strbuf either way, and we still have to handle
restoring the strbuf invariants. Even the strbuf_init() case is still
needed, because we don't know whether getdelim() just didn't allocate
(in which case we could leave the strbuf alone) or if it actually ate
the allocation we passed in (which was just a copy of sb->buf).

-Peff