{"thread":{"id":"65645","subject":"UBSan failing on expensive test(s)","startedAt":"2026-05-15T23:43:14Z","lastAt":"2026-05-16T02:55:51Z","messageCount":5,"participants":["Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"543429","messageId":"871pfcdyt0.fsf@gitster.g","threadId":"65645","inReplyTo":null,"subject":"UBSan failing on expensive test(s)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-15T23:43:07Z","receivedAt":"2026-05-15T23:43:14Z","isPatch":false,"body":"This started happening on 'next' that runs EXPENSIVE tests thanks to\nDscho's recent updates to enable them in CI.\n\nhttps://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172\n\nIt claims that \"\"\"\n\n    commit.c:1574:6: runtime error: signed integer overflow:\n    -2147483648 - 1 cannot be represented in type 'int'\n\n\"\"\".\n\nAnother is related in the sense that it used to be hidden behind\nEXPENSIVE prerequisite, but is probably unrelated.\n\nhttps://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156\n\n"},{"id":"543433","messageId":"20260516021343.GA174647@coredump.intra.peff.net","threadId":"65645","inReplyTo":"871pfcdyt0.fsf@gitster.g","subject":"Re: UBSan failing on expensive test(s)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-16T02:13:43Z","receivedAt":"2026-05-16T02:13:45Z","isPatch":false,"body":"On Sat, May 16, 2026 at 08:43:07AM +0900, Junio C Hamano wrote:\n\n> This started happening on 'next' that runs EXPENSIVE tests thanks to\n> Dscho's recent updates to enable them in CI.\n> \n> https://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172\n> \n> It claims that \"\"\"\n> \n>     commit.c:1574:6: runtime error: signed integer overflow:\n>     -2147483648 - 1 cannot be represented in type 'int'\n> \n> \"\"\".\n> \n> Another is related in the sense that it used to be hidden behind\n> EXPENSIVE prerequisite, but is probably unrelated.\n> \n> https://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156\n\nThese patches should fix both.\n\n  [1/2]: apply: plug leak on \"patch too large\" error\n  [2/2]: commit: handle large commit messages in utf8 verification\n\n apply.c  |  6 ++++--\n commit.c | 31 +++++++++++++++----------------\n 2 files changed, 19 insertions(+), 18 deletions(-)\n\n-Peff\n"},{"id":"543434","messageId":"20260516021622.GA744303@coredump.intra.peff.net","threadId":"65645","inReplyTo":"20260516021343.GA174647@coredump.intra.peff.net","subject":"[PATCH 1/2] apply: plug leak on \"patch too large\" error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-16T02:16:22Z","receivedAt":"2026-05-16T02:16:24Z","isPatch":true,"body":"In apply_patch(), we return immediately if read_patch_file() returns an\nerror. Traditionally this was OK, since an error from strbuf_read()\nwould restore the strbuf to its unallocated state.\n\nBut since f1c0e3946e (apply: reject patches larger than ~1 GiB,\n2022-10-25), we may also return an error if we successfully read the\npatch but it is too large. In this case we leak the strbuf contents when\napply_patch() returns.\n\nYou can see it in action by running t4141 under LSan with the EXPENSIVE\nprereq enabled.\n\nWe can fix this in one of two places:\n\n  1. In read_patch_file(), we could release the buffer before returning\n     the error, behaving more like a raw strbuf_read() call.\n\n  2. In apply_patch(), we can release the strbuf ourselves before\n     returning.\n\nI picked the latter, since it future proofs us against read_patch_file()\ngetting new error modes. We also have a cleanup label in that function\nalready, so now our error handling at this spot matches the rest of the\nfunction (and all of the variables are initialized such that the rest of\nthe cleanup is correctly a noop at this point).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n apply.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4aa1694cfa..249248d4f2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4881,8 +4881,10 @@ static int apply_patch(struct apply_state *state,\n \n \tstate->patch_input_file = filename;\n \tstate->linenr = 1;\n-\tif (read_patch_file(&buf, fd) < 0)\n-\t\treturn -128;\n+\tif (read_patch_file(&buf, fd) < 0) {\n+\t\tres = -128;\n+\t\tgoto end;\n+\t}\n \toffset = 0;\n \twhile (offset < buf.len) {\n \t\tstruct patch *patch;\n-- \n2.54.0.490.gaeb18d0c26\n\n"},{"id":"543435","messageId":"20260516022310.GB744303@coredump.intra.peff.net","threadId":"65645","inReplyTo":"20260516021343.GA174647@coredump.intra.peff.net","subject":"[PATCH 2/2] commit: handle large commit messages in utf8 verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-16T02:23:10Z","receivedAt":"2026-05-16T02:23:12Z","isPatch":true,"body":"Running t4205 under UBSan with the EXPENSIVE prereq enabled triggers an\nerror when we try to create a commit message that is over 2GB:\n\n  commit.c:1574:6: runtime error: signed integer overflow:\n    -2147483648 - 1 cannot be represented in type 'int'\n\nThe problem is that find_invalid_utf8() is not prepared to handle\nlarge buffers, as it uses an \"int\" to represent buffer sizes and\noffsets.\n\nWe can fix this with a few changes:\n\n  1. We'll take in \"len\" as a size_t (which is what the caller has\n     anyway, since it's working with a strbuf).\n\n  2. We need to return a size_t to give the offset to the invalid utf8,\n     but we also need a sentinel value for \"no invalid value\"\n     (previously \"-1\"). Let's split these to return a bool for \"found\n     invalid utf8\" and then pass back the offset as an out-parameter.\n     We'll switch the function name to match the new semantics.\n\n  3. The caller in verify_utf8() uses a \"long\" to store buffer\n     positions, which is a bit funny. This goes back to 08a94a145c\n     (commit/commit-tree: correct latin1 to utf-8, 2012-06-28) and is\n     perhaps trying to match our use of \"unsigned long\" for object sizes\n     (though we don't care about it ever becoming negative here). This\n     should be a size_t, too, as some platforms (like Windows) still use\n     a 32-bit long on machines with 64-bit pointers.\n\n  4. The \"bytes\" field within find_invalid_utf() does not have range\n     problems. It is the number of bytes the utf8 sequence claims to\n     have, so is limited by how many bits can be set in a single 8-bit\n     byte. However, if we leave it as an \"int\" then the compiler will\n     complain about the sign mismatch when comparing it to \"len\". So\n     let's make it unsigned, too.\n\nAll of this is a little silly, of course, because 2GB text commit\nmessages are clearly nonsense. So we might consider rejecting them\noutright, but it is easy enough to make these helper functions more\nrobust in the meantime.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI tried to look carefully for any reasons why these variables could ever\nbe negative, but beyond the sentinel value in the return type, didn't\nsee one. But reviewers should double check. Note that \"bad_offset =\noffset-1\" in the context looks suspicious, but we are guaranteed that\noffset has been advanced at this point.\n\n commit.c | 31 +++++++++++++++----------------\n 1 file changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 4385ae4329..8cf09be39d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1558,16 +1558,16 @@ int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,\n \treturn result;\n }\n \n-static int find_invalid_utf8(const char *buf, int len)\n+static bool has_invalid_utf8(const char *buf, size_t len, size_t *bad_offset)\n {\n-\tint offset = 0;\n+\tsize_t offset = 0;\n \tstatic const unsigned int max_codepoint[] = {\n \t\t0x7f, 0x7ff, 0xffff, 0x10ffff\n \t};\n \n \twhile (len) {\n \t\tunsigned char c = *buf++;\n-\t\tint bytes, bad_offset;\n+\t\tunsigned bytes;\n \t\tunsigned int codepoint;\n \t\tunsigned int min_val, max_val;\n \n@@ -1578,7 +1578,7 @@ static int find_invalid_utf8(const char *buf, int len)\n \t\tif (c < 0x80)\n \t\t\tcontinue;\n \n-\t\tbad_offset = offset-1;\n+\t\t*bad_offset = offset-1;\n \n \t\t/*\n \t\t * Count how many more high bits set: that's how\n@@ -1595,11 +1595,11 @@ static int find_invalid_utf8(const char *buf, int len)\n \t\t * codepoints beyond U+10FFFF, which are guaranteed never to exist.\n \t\t */\n \t\tif (bytes < 1 || 3 < bytes)\n-\t\t\treturn bad_offset;\n+\t\t\treturn true;\n \n \t\t/* Do we *have* that many bytes? */\n \t\tif (len < bytes)\n-\t\t\treturn bad_offset;\n+\t\t\treturn true;\n \n \t\t/*\n \t\t * Place the encoded bits at the bottom of the value and compute the\n@@ -1617,23 +1617,23 @@ static int find_invalid_utf8(const char *buf, int len)\n \t\t\tcodepoint <<= 6;\n \t\t\tcodepoint |= *buf & 0x3f;\n \t\t\tif ((*buf++ & 0xc0) != 0x80)\n-\t\t\t\treturn bad_offset;\n+\t\t\t\treturn true;\n \t\t} while (--bytes);\n \n \t\t/* Reject codepoints that are out of range for the sequence length. */\n \t\tif (codepoint < min_val || codepoint > max_val)\n-\t\t\treturn bad_offset;\n+\t\t\treturn true;\n \t\t/* Surrogates are only for UTF-16 and cannot be encoded in UTF-8. */\n \t\tif ((codepoint & 0x1ff800) == 0xd800)\n-\t\t\treturn bad_offset;\n+\t\t\treturn true;\n \t\t/* U+xxFFFE and U+xxFFFF are guaranteed non-characters. */\n \t\tif ((codepoint & 0xfffe) == 0xfffe)\n-\t\t\treturn bad_offset;\n+\t\t\treturn true;\n \t\t/* So are anything in the range U+FDD0..U+FDEF. */\n \t\tif (codepoint >= 0xfdd0 && codepoint <= 0xfdef)\n-\t\t\treturn bad_offset;\n+\t\t\treturn true;\n \t}\n-\treturn -1;\n+\treturn false;\n }\n \n /*\n@@ -1645,15 +1645,14 @@ static int find_invalid_utf8(const char *buf, int len)\n static int verify_utf8(struct strbuf *buf)\n {\n \tint ok = 1;\n-\tlong pos = 0;\n+\tsize_t pos = 0;\n \n \tfor (;;) {\n-\t\tint bad;\n+\t\tsize_t bad;\n \t\tunsigned char c;\n \t\tunsigned char replace[2];\n \n-\t\tbad = find_invalid_utf8(buf->buf + pos, buf->len - pos);\n-\t\tif (bad < 0)\n+\t\tif (!has_invalid_utf8(buf->buf + pos, buf->len - pos, &bad))\n \t\t\treturn ok;\n \t\tpos += bad;\n \t\tok = 0;\n-- \n2.54.0.490.gaeb18d0c26\n"},{"id":"543437","messageId":"256d83f3-dd0b-4a86-954a-de78ec65be6a@gmail.com","threadId":"65645","inReplyTo":"20260516021343.GA174647@coredump.intra.peff.net","subject":"Re: UBSan failing on expensive test(s)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-16T02:55:49Z","receivedAt":"2026-05-16T02:55:51Z","isPatch":false,"body":"On 5/15/26 10:13 PM, Jeff King wrote:\n> On Sat, May 16, 2026 at 08:43:07AM +0900, Junio C Hamano wrote:\n> \n>> This started happening on 'next' that runs EXPENSIVE tests thanks to\n>> Dscho's recent updates to enable them in CI.\n>>\n>> https://github.com/git/git/actions/runs/25896439353/job/76110441841#step:10:2172\n>>\n>> It claims that \"\"\"\n>>\n>>      commit.c:1574:6: runtime error: signed integer overflow:\n>>      -2147483648 - 1 cannot be represented in type 'int'\n>>\n>> \"\"\".\n>>\n>> Another is related in the sense that it used to be hidden behind\n>> EXPENSIVE prerequisite, but is probably unrelated.\n>>\n>> https://github.com/git/git/actions/runs/25896439353/job/76110441842#step:10:156\n> \n> These patches should fix both.\n> \n>    [1/2]: apply: plug leak on \"patch too large\" error\n>    [2/2]: commit: handle large commit messages in utf8 verification\nThanks for the fast fixes. I agree with you that 2GB commit\nmessages are silly, but the methods you change could be used\nfor other things so having better types is good. We can\nconsider blocking large commits as a feature another time.\n\n-Stolee\n\n"}]}