patchpretty: drop strbuf pre-sizing from add_rfc2047()

3 messages between May 12, 2026 and May 13, 2026, from Jeff King, Junio C Hamano.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Jeff KingMay 12, 2026, 16:20 UTC on lore

At the top of add_rfc2047() we do this:

  strbuf_grow(sb, len * 3 + strlen(encoding) + 100);

where "len" is the size of the header (like an author name) we are about to encode into the buffer. This pre-sizing is purely an optimization; we use strbuf_addf() and friends to actually write into the buffer, and they will grow the buffer as necessary.

But there's a problem with the code above: the input can be arbitrarily large, so we might overflow a size_t while doing that computation, ending up with a too-small allocation request. Overflowing requires an impractically large input on a 64-bit system, but is easy to demonstrate on a 32-bit system with a commit whose author name is ~1.4GB.

Because this pre-sizing is just an optimization, there's no real harm. We'll start with a smaller buffer and grow it as necessary. But it _looks_ like a vulnerability, since some other code may pre-size a strbuf and then write directly into its buffer. So it's worth avoiding the overflow in the first place.

The obvious way to do that is via checked operations like st_add() and friends. But taking a step back, is this pre-sizing actually helping anything?

The computation goes all the way back to 4234a76167 (Extend --pretty=oneline to cover the first paragraph,, 2007-06-11), but back then we really were sizing the array to write into directly! In 674d172730 (Rework pretty_print_commit to use strbufs instead of custom buffers., 2007-09-10) that switched to a strbuf, and at that point it was a pure optimization.

Is the optimization helping? I don't think so. Even for a gigantic case like the 1.4GB author name, I couldn't measure any slowdown when removing it. And most input will be much smaller, and added to a running strbuf containing the rest of the email-header output. We can just rely on strbuf's usual amortized-linear growth.

So deleting the line seems like the best way to go. It eliminates the integer overflow and makes the code a tiny bit simpler.

Reported-by: Luke Martin <lmartin@paramenoeng.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 pretty.c | 1 -
 1 file changed, 1 deletion(-)

Show changes to pretty.c +0 −1

diff --git a/pretty.c b/pretty.c
index 814803980b..7328aecf5d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -399,7 +399,6 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,
 	int i;
 	int line_len = last_line_length(sb);
 
-	strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
 	strbuf_addf(sb, "=?%s?q?", encoding);
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */

-- 
2.54.0.420.gf0bcdff42b

Junio C HamanoMay 13, 2026, 01:03 UTC in reply to Jeff King on lore

Re: [PATCH] pretty: drop strbuf pre-sizing from add_rfc2047()

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> At the top of add_rfc2047() we do this:
>
>   strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
>
> where "len" is the size of the header (like an author name) we are about
> to encode into the buffer. This pre-sizing is purely an optimization; we
> use strbuf_addf() and friends to actually write into the buffer, and
> they will grow the buffer as necessary.
> ...
> Is the optimization helping? I don't think so. Even for a gigantic case
> like the 1.4GB author name, I couldn't measure any slowdown when
> removing it. And most input will be much smaller, and added to a running
> strbuf containing the rest of the email-header output. We can just rely
> on strbuf's usual amortized-linear growth.
>
> So deleting the line seems like the best way to go. It eliminates the
> integer overflow and makes the code a tiny bit simpler.

Very nice.

Someday we may want to go through the output from

    $ git grep -e 'strbuf_grow(' \*.c

and remove this ineffective presizing. I think any call to strbuf_grow() that is immediately followed by a call to strbuf_addX() is suspect, like in the following illustration (there are others in these files).

Show changes to diff +0 −2

diff --git c/apply.c w/apply.c
index 3de4aa4d2e..b0f146276e 100644
--- c/apply.c
+++ w/apply.c
@@ -3305,7 +3305,6 @@ static int apply_fragments(struct apply_state *state, struct image *img, struct
 static int read_blob_object(struct strbuf *buf, const struct object_id *oid, unsigned mode)
 {
 	if (S_ISGITLINK(mode)) {
-		strbuf_grow(buf, 100);
 		strbuf_addf(buf, "Subproject commit %s\n", oid_to_hex(oid));
 	} else {
 		enum object_type type;
diff --git c/archive.c w/archive.c
index fcd474c682..47b8725f0a 100644
--- c/archive.c
+++ w/archive.c
@@ -164,7 +164,6 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
 
 	args->convert = 0;
 	strbuf_reset(&path);
-	strbuf_grow(&path, PATH_MAX);
 	strbuf_add(&path, args->base, args->baselen);
 	strbuf_add(&path, base, baselen);
 	strbuf_addstr(&path, filename);

Jeff KingMay 13, 2026, 18:54 UTC in reply to Junio C Hamano on lore

Re: [PATCH] pretty: drop strbuf pre-sizing from add_rfc2047()

On Wed, May 13, 2026 at 10:03:31AM +0900, Junio C Hamano wrote:

Show 8 quoted lines

> Someday we may want to go through the output from
> 
>     $ git grep -e 'strbuf_grow(' \*.c
> 
> and remove this ineffective presizing.  I think any call to
> strbuf_grow() that is immediately followed by a call to
> strbuf_addX() is suspect, like in the following illustration (there
> are others in these files).

Yup. I think this could be #leftoverbits material, but anybody who wants to pick this up should be careful to read through the whole function and make sure there's no subtle dependency on the grown buffer.

Skimming through, it looks like most are just leftovers from when old code was converted to strbuf, and the pre-growth was kept mostly out of conservatism.

Some of them are truly ugly to look at, like:

  http-backend.c: strbuf_grow(&buf, cnt * 53 + 2);

and I think in some cases we can even drop some surrounding code, like:

Show changes to builtin/fast-import.c +0 −8

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 070a5af3e4..aa32ebc8ab 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1337,21 +1337,14 @@ static int tecmp1 (const void *_a, const void *_b)
 
 static void mktree(struct tree_content *t, int v, struct strbuf *b)
 {
-	size_t maxlen = 0;
 	unsigned int i;
 
 	if (!v)
 		QSORT(t->entries, t->entry_count, tecmp0);
 	else
 		QSORT(t->entries, t->entry_count, tecmp1);
 
-	for (i = 0; i < t->entry_count; i++) {
-		if (t->entries[i]->versions[v].mode)
-			maxlen += t->entries[i]->name->str_len + 34;
-	}
-
 	strbuf_reset(b);
-	strbuf_grow(b, maxlen);
 	for (i = 0; i < t->entry_count; i++) {
 		struct tree_entry *e = t->entries[i];
 		if (!e->versions[v].mode)

So probably some satisfying cleanup opportunities available for somebody
who wants to spend a little time with it. ;)

-Peff