{"thread":{"id":"51485","subject":"git segfault in tag verify (patch included)","startedAt":"2019-07-16T17:47:56Z","lastAt":"2019-07-16T19:18:00Z","messageCount":5,"participants":["Steven Roberts","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"378981","messageId":"CAOGP0GoYviFHhFzhqEimEw+ebd=DJx9JucG4BNvSvPuDudoWxw@mail.gmail.com","threadId":"51485","inReplyTo":null,"subject":"git segfault in tag verify (patch included)","fromName":"Steven Roberts","fromEmail":"fenderq@gmail.com","sentAt":"2019-07-16T17:47:17Z","receivedAt":"2019-07-16T17:47:56Z","isPatch":false,"sender":{"key":"fenderq@gmail.com","avatar":null},"body":"Hi,\n\nI believe I have found an off-by-one error in git.\n\nPlease see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2\n"},{"id":"378984","messageId":"xmqqsgr53kov.fsf@gitster-ct.c.googlers.com","threadId":"51485","inReplyTo":"CAOGP0GoYviFHhFzhqEimEw+ebd=DJx9JucG4BNvSvPuDudoWxw@mail.gmail.com","subject":"Re: git segfault in tag verify (patch included)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-16T18:20:48Z","receivedAt":"2019-07-16T18:20:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steven Roberts writes:\n\n> I believe I have found an off-by-one error in git.\n>\n> Please see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2\n\nThat is this thing.\n\n static void parse_gpg_output(struct signature_check *sigc)\n {\n const char *buf = sigc->gpg_status;\n const char *line, *next;\n int i, j;\n int seen_exclusive_status = 0;\n\n /* Iterate over all lines */\n for (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n while (*line == '\\n')\n line++;\n /* Skip lines that don't start with GNUPG status */\n if (!skip_prefix(line, \"[GNUPG:] \", &line))\n continue;\n\nIf the GPG output ends with a trailing blank line, we skip and get\nto the terminating NUL, then find that it does not begin with\nthe \"[GNUPG:] \" prefix, and hit the continue. We try to scan and\nlook for LF (or stop at the end of the string) for the next round,\nstarting at one past where we are, which is already the terminating\nNUL. Ouch.\n\nGood finding.\n\nWe need your sign-off (see Documentation/SubmittingPatches).\n\nThanks.\n\n\n-- >8 --\nFrom: Steven Roberts \nSubject: gpg-interface: do not scan past the end of buffer\n\nIf the GPG output ends with trailing blank lines, after skipping\nthem over inside the loop to find the terminating NUL at the end,\nthe loop ends up looking for the next line, starting past the end.\n\n---\n gpg-interface.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 8ed274533f..eb55d46ea4 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -116,6 +116,9 @@ static void parse_gpg_output(struct signature_check *sigc)\n \tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n \t\twhile (*line == '\\n')\n \t\t\tline++;\n+\t\tif (!*line)\n+\t\t\tbreak;\n+\n \t\t/* Skip lines that don't start with GNUPG status */\n \t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n \t\t\tcontinue;\n\n"},{"id":"378986","messageId":"CAOGP0Gp6zUVeBZjMaCuoHcoHhGJZyC1T1SFNtZSDTL1rXA8eCw@mail.gmail.com","threadId":"51485","inReplyTo":"xmqqsgr53kov.fsf@gitster-ct.c.googlers.com","subject":"Re: git segfault in tag verify (patch included)","fromName":"Steven Roberts","fromEmail":"fenderq@gmail.com","sentAt":"2019-07-16T18:47:37Z","receivedAt":"2019-07-16T18:48:16Z","isPatch":false,"sender":{"key":"fenderq@gmail.com","avatar":null},"body":"Thanks. I hope this works ok for you (see attached).\n\nOn Tue, Jul 16, 2019 at 11:20 AM Junio C Hamano wrote:\n>\n> Steven Roberts writes:\n>\n> > I believe I have found an off-by-one error in git.\n> >\n> > Please see https://marc.info/?l=openbsd-ports&m=156326783610123&w=2\n>\n> That is this thing.\n>\n> static void parse_gpg_output(struct signature_check *sigc)\n> {\n> const char *buf = sigc->gpg_status;\n> const char *line, *next;\n> int i, j;\n> int seen_exclusive_status = 0;\n>\n> /* Iterate over all lines */\n> for (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n> while (*line == '\\n')\n> line++;\n> /* Skip lines that don't start with GNUPG status */\n> if (!skip_prefix(line, \"[GNUPG:] \", &line))\n> continue;\n>\n> If the GPG output ends with a trailing blank line, we skip and get\n> to the terminating NUL, then find that it does not begin with\n> the \"[GNUPG:] \" prefix, and hit the continue. We try to scan and\n> look for LF (or stop at the end of the string) for the next round,\n> starting at one past where we are, which is already the terminating\n> NUL. Ouch.\n>\n> Good finding.\n>\n> We need your sign-off (see Documentation/SubmittingPatches).\n>\n> Thanks.\n>\n>\n> -- >8 --\n> From: Steven Roberts \n> Subject: gpg-interface: do not scan past the end of buffer\n>\n> If the GPG output ends with trailing blank lines, after skipping\n> them over inside the loop to find the terminating NUL at the end,\n> the loop ends up looking for the next line, starting past the end.\n>\n> ---\n> gpg-interface.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 8ed274533f..eb55d46ea4 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -116,6 +116,9 @@ static void parse_gpg_output(struct signature_check *sigc)\n> for (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n> while (*line == '\\n')\n> line++;\n> + if (!*line)\n> + break;\n> +\n> /* Skip lines that don't start with GNUPG status */\n> if (!skip_prefix(line, \"[GNUPG:] \", &line))\n> continue;\n>\n\n\n-- \nSteven Roberts | https://www.fenderq.com/\n\n\nFrom d48814273a50cf0b293148cc40a6a5cc7c13686e Mon Sep 17 00:00:00 2001\nFrom: Steven Roberts \nDate: Tue, 16 Jul 2019 11:40:46 -0700\nSubject: [PATCH] gpg-interface: do not scan past the end of buffer\n\nSigned-off-by: Steven Roberts \n---\n gpg-interface.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 8ed274533f..775475131d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -116,6 +116,11 @@ static void parse_gpg_output(struct signature_check *sigc)\n \tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n \t\twhile (*line == '\\n')\n \t\t\tline++;\n+\n+\t\t/* Break out of trailing '\\n' */\n+\t\tif (!*line)\n+\t\t\tbreak;\n+\n \t\t/* Skip lines that don't start with GNUPG status */\n \t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n \t\t\tcontinue;\n-- \n2.21.0\n\n"},{"id":"378987","messageId":"20190716185835.GA24468@sigill.intra.peff.net","threadId":"51485","inReplyTo":"xmqqsgr53kov.fsf@gitster-ct.c.googlers.com","subject":"Re: git segfault in tag verify (patch included)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-16T18:58:35Z","receivedAt":"2019-07-16T18:58:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 16, 2019 at 11:20:48AM -0700, Junio C Hamano wrote:\n\n> That is this thing.\n> \n> static void parse_gpg_output(struct signature_check *sigc)\n> {\n> const char *buf = sigc->gpg_status;\n> const char *line, *next;\n> int i, j;\n> int seen_exclusive_status = 0;\n> \n> /* Iterate over all lines */\n> for (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n> while (*line == '\\n')\n> line++;\n> /* Skip lines that don't start with GNUPG status */\n> if (!skip_prefix(line, \"[GNUPG:] \", &line))\n> continue;\n> \n> If the GPG output ends with a trailing blank line, we skip and get\n> to the terminating NUL, then find that it does not begin with\n> the \"[GNUPG:] \" prefix, and hit the continue. We try to scan and\n> look for LF (or stop at the end of the string) for the next round,\n> starting at one past where we are, which is already the terminating\n> NUL. Ouch.\n> \n> Good finding.\n\nYeah. The patch below looks fine, and I do not see any other\nout-of-bounds issues in the code (there is a similar \"next + 1\" in the\ninner loop, but it checks for an empty line right beforehand already).\n\nI find these kind of \"+1\" pointer increments without constraint checking\nare error-prone. I suspect this whole loop could be a bit easier to\nfollow by finding the next line boundary at the start of the loop, and\njumping to it in the for-loop advancement. And then within the loop, we\nknow the boundaries of the line (right now, for example, we issue a\nstrchrnul() looking for a space that might unexpectedly cross line\nboundaries).\n\nSomething like:\n\n for (line = buf; *line; line = next) {\n\tnext = strchrnul(line, '\\n');\n\n\t... do stuff ...\n\t/* find a space within the line */\n\tspace = memchr(line, ' ', next - line);\n }\n\nor even:\n\n for (line = buf; *line; line += len) {\n\tsize_t len = strchrnul(line, '\\n') - line;\n\t...\n\tspace = memchr(line, ' ', linelen);\n }\n\nbut it may not be worth the churn. It's just as likely to introduce a\nnew bug. ;)\n\nI've also found working with strbuf_getline() to be very pleasant for a\nlot of these cases (in which you get a real per-line NUL-terminated\nstring), but of course it implies an extra copy.\n\n-Peff\n"},{"id":"378989","messageId":"xmqqk1ch3i1s.fsf@gitster-ct.c.googlers.com","threadId":"51485","inReplyTo":"20190716185835.GA24468@sigill.intra.peff.net","subject":"Re: git segfault in tag verify (patch included)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-16T19:17:51Z","receivedAt":"2019-07-16T19:18:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Something like:\n>\n> for (line = buf; *line; line = next) {\n> \tnext = strchrnul(line, '\\n');\n>\n> \t... do stuff ...\n> \t/* find a space within the line */\n> \tspace = memchr(line, ' ', next - line);\n> }\n\nI am not sure about the memchr() thing, but \"prepare next that is\nconstant thru the iteration, and update line to it at the end of\neach iteration\" is a robust pattern to follow. I like it.\n\n"}]}