{"thread":{"id":"41625","subject":"[PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error","startedAt":"2016-03-05T18:43:30Z","lastAt":"2016-03-05T19:36:41Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280254","messageId":"20160305184330.GA7534@sigill.intra.peff.net","threadId":"41625","inReplyTo":null,"subject":"[PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T18:43:30Z","receivedAt":"2016-03-05T18:43:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 0cc30e0 (strbuf_getwholeline: use getdelim if it is\navailable, 2015-04-16) tries to clean up after getdelim()\nreturns EOF, but gets one case wrong, which can lead in some\nobscure cases to us reading uninitialized memory.\n\nAfter getdelim() returns -1, we re-initialize the strbuf\nonly if sb->buf is NULL. The thinking was that either:\n\n  1. We fed an existing allocated buffer to getdelim(), and\n     at most it would have realloc'd, leaving our NUL in\n     place.\n\n  2. We didn't have a buffer to feed, so we gave getdelim()\n     NULL; sb->buf will remain NULL, and we just want to\n     restore the empty slopbuf.\n\nBut that second case isn't quite right. getdelim() may\nallocate a buffer, write nothing into it, and then return\nEOF. The resulting strbuf rightfully has sb->len set to \"0\",\nbut is missing the NUL terminator in the first byte.\n\nMost call-sites are fine with this. They see the EOF and\ndon't bother looking at the strbuf. Or they notice that\nsb->len is empty, and don't look at the contents. But\nthere's at least one case that does neither, and relies on\nparsing the resulting (possibly zero-length) string:\nfast-import. You can see this in action with the new test\n(though we probably only notice failure there when run with\n--valgrind or ASAN).\n\nWe can fix this by unconditionally resetting the strbuf when\nwe have a buffer after getdelim(). That fixes case 2 above.\nCase 1 is probably already fine in practice, but it does not\nhurt for us to re-assert our invariants (especially because\nwe are relying on whatever getdelim() happens to do, which\nmay vary from platform to platform). Our fix covers that\ncase, too.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNot a big rush for 2.8.0-rc, as the bug is in v2.5.0, and I doubt\nthere's an easy trigger besides fast-import. But it might be harmless\nenough to squeeze in.\n\n strbuf.c               | 8 +++++++-\n t/t9300-fast-import.sh | 4 ++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex f60e2ee..2c08dbb 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -481,9 +481,15 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \tif (errno == ENOMEM)\n \t\tdie(\"Out of memory, getdelim failed\");\n \n-\t/* Restore slopbuf that we moved out of the way before */\n+\t/*\n+\t * Restore strbuf invariants; if getdelim left us with a NULL pointer,\n+\t * we can just re-init, but otherwise we should make sure that our\n+\t * length is empty, and that the result is NUL-terminated.\n+\t */\n \tif (!sb->buf)\n \t\tstrbuf_init(sb, 0);\n+\telse\n+\t\tstrbuf_reset(sb);\n \treturn EOF;\n }\n #else\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 4c5f3c9..25bb60b 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -55,6 +55,10 @@ test_expect_success 'empty stream succeeds' '\n \tgit fast-import </dev/null\n '\n \n+test_expect_success 'truncated stream complains' '\n+\techo \"tag foo\" | test_must_fail git fast-import\n+'\n+\n test_expect_success 'A: create pack from stdin' '\n \ttest_tick &&\n \tcat >input <<-INPUT_END &&\n-- \n2.8.0.rc1.318.g2193183\n"},{"id":"280257","messageId":"xmqqziucu3d7.fsf@gitster.mtv.corp.google.com","threadId":"41625","inReplyTo":"20160305184330.GA7534@sigill.intra.peff.net","subject":"Re: [PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-05T19:33:08Z","receivedAt":"2016-03-05T19:33:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Not a big rush for 2.8.0-rc, as the bug is in v2.5.0, and I doubt\n> there's an easy trigger besides fast-import. But it might be harmless\n> enough to squeeze in.\n\nWas it found by a real-world debugging session, or by a code\ninspection?  I'd be really impressed if it were the latter ;-)\n\nThanks, will queue.\n\n>  strbuf.c               | 8 +++++++-\n>  t/t9300-fast-import.sh | 4 ++++\n>  2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index f60e2ee..2c08dbb 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -481,9 +481,15 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n>  \tif (errno == ENOMEM)\n>  \t\tdie(\"Out of memory, getdelim failed\");\n>  \n> -\t/* Restore slopbuf that we moved out of the way before */\n> +\t/*\n> +\t * Restore strbuf invariants; if getdelim left us with a NULL pointer,\n> +\t * we can just re-init, but otherwise we should make sure that our\n> +\t * length is empty, and that the result is NUL-terminated.\n> +\t */\n>  \tif (!sb->buf)\n>  \t\tstrbuf_init(sb, 0);\n> +\telse\n> +\t\tstrbuf_reset(sb);\n>  \treturn EOF;\n>  }\n>  #else\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 4c5f3c9..25bb60b 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -55,6 +55,10 @@ test_expect_success 'empty stream succeeds' '\n>  \tgit fast-import </dev/null\n>  '\n>  \n> +test_expect_success 'truncated stream complains' '\n> +\techo \"tag foo\" | test_must_fail git fast-import\n> +'\n> +\n>  test_expect_success 'A: create pack from stdin' '\n>  \ttest_tick &&\n>  \tcat >input <<-INPUT_END &&\n"},{"id":"280260","messageId":"20160305193640.GA20701@sigill.intra.peff.net","threadId":"41625","inReplyTo":"xmqqziucu3d7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T19:36:41Z","receivedAt":"2016-03-05T19:36:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 05, 2016 at 11:33:08AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Not a big rush for 2.8.0-rc, as the bug is in v2.5.0, and I doubt\n> > there's an easy trigger besides fast-import. But it might be harmless\n> > enough to squeeze in.\n> \n> Was it found by a real-world debugging session, or by a code\n> inspection?  I'd be really impressed if it were the latter ;-)\n\nSorry to disappoint, but it was the former. :)\n\nI found (and am fixing) a _different_ bug with:\n\n  {\n\techo \"tag foo\"\n\techo \"from HEAD:./bar\"\n  } | git fast-import\n\nand I wondered if fast-import would take:\n\n  echo \"tag foo from HEAD:./bar\"\n\non one line.  The answer is no, btw, but it also tickles this bug (it is\nexpecting \"from ...\" on the next line, and handles EOF by detecting the\nempty string).\n\n-Peff\n"}]}