{"thread":{"id":"15089","subject":"[PATCH] mailinfo: avoid violating strbuf assertion","startedAt":"2008-08-19T17:28:24Z","lastAt":"2008-08-20T18:34:59Z","messageCount":2,"participants":["Jeff King","Don Zickus"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"87716","messageId":"20080819172824.GA9886@coredump.intra.peff.net","threadId":"15089","inReplyTo":null,"subject":"[PATCH] mailinfo: avoid violating strbuf assertion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-19T17:28:24Z","receivedAt":"2008-08-19T17:28:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In handle_from, we calculate the end boundary of a section\nto remove from a strbuf using strcspn like this:\n\n el = strcspn(buf, set_of_end_boundaries);\n strbuf_remove(&sb, start, el + 1);\n\nThis works fine if \"el\" is the offset of the boundary\ncharacter, meaning we remove up to and including that\ncharacter. But if the end boundary didn't match (that is, we\nhit the end of the string as the boundary instead) then we\nwant just \"el\". Asking for \"el+1\" caught an out-of-bounds\nassertion in the strbuf library.\n\nThis manifested itself when we got a 'From' header that had\njust an email address with nothing else in it (the end of\nthe string was the end of the address, rather than, e.g., a\ntrailing '>' character), causing git-mailinfo to barf.\n\nSigned-off-by: Jeff King \n---\n> It's the From header actually. The patch below should fix\n> it (though it sure makes that line of code ugly --\n> improvements are welcome).\n\nI thought it would be more readable to conditionally\nincrement 'el' when we set, but we actually need it in the\nunincremented form on the previous line. So this is the fix\nthat I posted before, but with a test and a few commit\nmessage cleanups.\n\n builtin-mailinfo.c | 2 +-\n t/t5100-mailinfo.sh | 11 +++++++++++\n t/t5100/info-from.expect | 5 +++++\n t/t5100/info-from.in | 8 ++++++++\n 4 files changed, 25 insertions(+), 1 deletions(-)\n create mode 100644 t/t5100/info-from.expect\n create mode 100644 t/t5100/info-from.in\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 26d3e5d..e890f7a 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -107,7 +107,7 @@ static void handle_from(const struct strbuf *from)\n \tel = strcspn(at, \" \\n\\t\\r\\v\\f>\");\n \tstrbuf_reset(&email);\n \tstrbuf_add(&email, at, el);\n-\tstrbuf_remove(&f, at - f.buf, el + 1);\n+\tstrbuf_remove(&f, at - f.buf, el + (at[el] ? 1 : 0));\n \n \t/* The remainder is name. It could be \"John Doe \"\n \t * or \"john.doe@xz (John Doe)\", but we have removed the\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 3b6c3a9..fe14589 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -44,4 +44,15 @@ test_expect_success 'Preserve NULs out of MIME encoded message' '\n \n '\n \n+test_expect_success 'mailinfo on from header without name works' '\n+\n+\tmkdir info-from &&\n+\tgit mailsplit -oinfo-from \"$TEST_DIRECTORY\"/t5100/info-from.in &&\n+\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.in info-from/0001 &&\n+\tgit mailinfo info-from/msg info-from/patch \\\n+\t info-from/out &&\n+\ttest_cmp \"$TEST_DIRECTORY\"/t5100/info-from.expect info-from/out\n+\n+'\n+\n test_done\ndiff --git a/t/t5100/info-from.expect b/t/t5100/info-from.expect\nnew file mode 100644\nindex 0000000..c31d2eb\n--- /dev/null\n+++ b/t/t5100/info-from.expect\n@@ -0,0 +1,5 @@\n+Author: bare@example.com\n+Email: bare@example.com\n+Subject: testing bare address in from header\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+\ndiff --git a/t/t5100/info-from.in b/t/t5100/info-from.in\nnew file mode 100644\nindex 0000000..4f08209\n--- /dev/null\n+++ b/t/t5100/info-from.in\n@@ -0,0 +1,8 @@\n+From 667d8940e719cddee1cfe237cbbe215e20270b09 Mon Sep 17 00:00:00 2001\n+From: bare@example.com\n+Date: Sun, 25 May 2008 00:38:18 -0700\n+Subject: [PATCH] testing bare address in from header\n+\n+commit message\n+---\n+patch\n-- \n1.6.0.96.g2fad1.dirty\n"},{"id":"87842","messageId":"20080820183459.GA26052@redhat.com","threadId":"15089","inReplyTo":"20080819172824.GA9886@coredump.intra.peff.net","subject":"Re: [PATCH] mailinfo: avoid violating strbuf assertion","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-08-20T18:34:59Z","receivedAt":"2008-08-20T18:34:59Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"On Tue, Aug 19, 2008 at 01:28:24PM -0400, Jeff King wrote:\n> In handle_from, we calculate the end boundary of a section\n> to remove from a strbuf using strcspn like this:\n> \n> el = strcspn(buf, set_of_end_boundaries);\n> strbuf_remove(&sb, start, el + 1);\n> \n> This works fine if \"el\" is the offset of the boundary\n> character, meaning we remove up to and including that\n> character. But if the end boundary didn't match (that is, we\n> hit the end of the string as the boundary instead) then we\n> want just \"el\". Asking for \"el+1\" caught an out-of-bounds\n> assertion in the strbuf library.\n> \n> This manifested itself when we got a 'From' header that had\n> just an email address with nothing else in it (the end of\n> the string was the end of the address, rather than, e.g., a\n> trailing '>' character), causing git-mailinfo to barf.\n\nOdd, I just ran into this myself today too. Wonder if we share the same\nculprit.. :-)\n\nTested-by: Don Zickus \n"}]}