{"thread":{"id":"65624","subject":"[PATCH] pretty: drop strbuf pre-sizing from add_rfc2047()","startedAt":"2026-05-12T16:20:25Z","lastAt":"2026-05-13T18:54:15Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543191","messageId":"20260512162022.GA69669@coredump.intra.peff.net","threadId":"65624","inReplyTo":null,"subject":"[PATCH] pretty: drop strbuf pre-sizing from add_rfc2047()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-12T16:20:22Z","receivedAt":"2026-05-12T16:20:25Z","isPatch":true,"body":"At the top of add_rfc2047() we do this:\n\n  strbuf_grow(sb, len * 3 + strlen(encoding) + 100);\n\nwhere \"len\" is the size of the header (like an author name) we are about\nto encode into the buffer. This pre-sizing is purely an optimization; we\nuse strbuf_addf() and friends to actually write into the buffer, and\nthey will grow the buffer as necessary.\n\nBut there's a problem with the code above: the input can be arbitrarily\nlarge, so we might overflow a size_t while doing that computation,\nending up with a too-small allocation request. Overflowing requires an\nimpractically large input on a 64-bit system, but is easy to demonstrate\non a 32-bit system with a commit whose author name is ~1.4GB.\n\nBecause this pre-sizing is just an optimization, there's no real harm.\nWe'll start with a smaller buffer and grow it as necessary. But it\n_looks_ like a vulnerability, since some other code may pre-size a\nstrbuf and then write directly into its buffer. So it's worth avoiding\nthe overflow in the first place.\n\nThe obvious way to do that is via checked operations like st_add() and\nfriends. But taking a step back, is this pre-sizing actually helping\nanything?\n\nThe computation goes all the way back to 4234a76167 (Extend\n--pretty=oneline to cover the first paragraph,, 2007-06-11), but back\nthen we really were sizing the array to write into directly! In\n674d172730 (Rework pretty_print_commit to use strbufs instead of custom\nbuffers., 2007-09-10) that switched to a strbuf, and at that point it\nwas a pure optimization.\n\nIs the optimization helping? I don't think so. Even for a gigantic case\nlike the 1.4GB author name, I couldn't measure any slowdown when\nremoving it. And most input will be much smaller, and added to a running\nstrbuf containing the rest of the email-header output. We can just rely\non strbuf's usual amortized-linear growth.\n\nSo deleting the line seems like the best way to go. It eliminates the\ninteger overflow and makes the code a tiny bit simpler.\n\nReported-by: Luke Martin <lmartin@paramenoeng.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pretty.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 814803980b..7328aecf5d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -399,7 +399,6 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,\n \tint i;\n \tint line_len = last_line_length(sb);\n \n-\tstrbuf_grow(sb, len * 3 + strlen(encoding) + 100);\n \tstrbuf_addf(sb, \"=?%s?q?\", encoding);\n \tline_len += strlen(encoding) + 5; /* 5 for =??q? */\n \n-- \n2.54.0.420.gf0bcdff42b\n"},{"id":"543223","messageId":"xmqqtsscjf30.fsf@gitster.g","threadId":"65624","inReplyTo":"20260512162022.GA69669@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: drop strbuf pre-sizing from add_rfc2047()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-13T01:03:31Z","receivedAt":"2026-05-13T01:03:35Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> At the top of add_rfc2047() we do this:\n>\n>   strbuf_grow(sb, len * 3 + strlen(encoding) + 100);\n>\n> where \"len\" is the size of the header (like an author name) we are about\n> to encode into the buffer. This pre-sizing is purely an optimization; we\n> use strbuf_addf() and friends to actually write into the buffer, and\n> they will grow the buffer as necessary.\n> ...\n> Is the optimization helping? I don't think so. Even for a gigantic case\n> like the 1.4GB author name, I couldn't measure any slowdown when\n> removing it. And most input will be much smaller, and added to a running\n> strbuf containing the rest of the email-header output. We can just rely\n> on strbuf's usual amortized-linear growth.\n>\n> So deleting the line seems like the best way to go. It eliminates the\n> integer overflow and makes the code a tiny bit simpler.\n\nVery nice.\n\nSomeday we may want to go through the output from\n\n    $ git grep -e 'strbuf_grow(' \\*.c\n\nand remove this ineffective presizing.  I think any call to\nstrbuf_grow() that is immediately followed by a call to\nstrbuf_addX() is suspect, like in the following illustration (there\nare others in these files).\n\ndiff --git c/apply.c w/apply.c\nindex 3de4aa4d2e..b0f146276e 100644\n--- c/apply.c\n+++ w/apply.c\n@@ -3305,7 +3305,6 @@ static int apply_fragments(struct apply_state *state, struct image *img, struct\n static int read_blob_object(struct strbuf *buf, const struct object_id *oid, unsigned mode)\n {\n \tif (S_ISGITLINK(mode)) {\n-\t\tstrbuf_grow(buf, 100);\n \t\tstrbuf_addf(buf, \"Subproject commit %s\\n\", oid_to_hex(oid));\n \t} else {\n \t\tenum object_type type;\ndiff --git c/archive.c w/archive.c\nindex fcd474c682..47b8725f0a 100644\n--- c/archive.c\n+++ w/archive.c\n@@ -164,7 +164,6 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \n \targs->convert = 0;\n \tstrbuf_reset(&path);\n-\tstrbuf_grow(&path, PATH_MAX);\n \tstrbuf_add(&path, args->base, args->baselen);\n \tstrbuf_add(&path, base, baselen);\n \tstrbuf_addstr(&path, filename);\n\n\n\n\n\n"},{"id":"543259","messageId":"20260513185408.GA147423@coredump.intra.peff.net","threadId":"65624","inReplyTo":"xmqqtsscjf30.fsf@gitster.g","subject":"Re: [PATCH] pretty: drop strbuf pre-sizing from add_rfc2047()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-13T18:54:08Z","receivedAt":"2026-05-13T18:54:15Z","isPatch":true,"body":"On Wed, May 13, 2026 at 10:03:31AM +0900, Junio C Hamano wrote:\n\n> Someday we may want to go through the output from\n> \n>     $ git grep -e 'strbuf_grow(' \\*.c\n> \n> and remove this ineffective presizing.  I think any call to\n> strbuf_grow() that is immediately followed by a call to\n> strbuf_addX() is suspect, like in the following illustration (there\n> are others in these files).\n\nYup. I think this could be #leftoverbits material, but anybody who wants\nto pick this up should be careful to read through the whole function and\nmake sure there's no subtle dependency on the grown buffer.\n\nSkimming through, it looks like most are just leftovers from when old\ncode was converted to strbuf, and the pre-growth was kept mostly out of\nconservatism.\n\nSome of them are truly ugly to look at, like:\n\n  http-backend.c: strbuf_grow(&buf, cnt * 53 + 2);\n\nand I think in some cases we can even drop some surrounding code, like:\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 070a5af3e4..aa32ebc8ab 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1337,21 +1337,14 @@ static int tecmp1 (const void *_a, const void *_b)\n \n static void mktree(struct tree_content *t, int v, struct strbuf *b)\n {\n-\tsize_t maxlen = 0;\n \tunsigned int i;\n \n \tif (!v)\n \t\tQSORT(t->entries, t->entry_count, tecmp0);\n \telse\n \t\tQSORT(t->entries, t->entry_count, tecmp1);\n \n-\tfor (i = 0; i < t->entry_count; i++) {\n-\t\tif (t->entries[i]->versions[v].mode)\n-\t\t\tmaxlen += t->entries[i]->name->str_len + 34;\n-\t}\n-\n \tstrbuf_reset(b);\n-\tstrbuf_grow(b, maxlen);\n \tfor (i = 0; i < t->entry_count; i++) {\n \t\tstruct tree_entry *e = t->entries[i];\n \t\tif (!e->versions[v].mode)\n\nSo probably some satisfying cleanup opportunities available for somebody\nwho wants to spend a little time with it. ;)\n\n-Peff\n"}]}