[PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error

[PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error

[Jeff King]

Commit 0cc30e0 (strbuf_getwholeline: use getdelim if it is
available, 2015-04-16) tries to clean up after getdelim()
returns EOF, but gets one case wrong, which can lead in some
obscure cases to us reading uninitialized memory.

After getdelim() returns -1, we re-initialize the strbuf
only if sb->buf is NULL. The thinking was that either:

  1. We fed an existing allocated buffer to getdelim(), and
     at most it would have realloc'd, leaving our NUL in
     place.

  2. We didn't have a buffer to feed, so we gave getdelim()
     NULL; sb->buf will remain NULL, and we just want to
     restore the empty slopbuf.

But that second case isn't quite right. getdelim() may
allocate a buffer, write nothing into it, and then return
EOF. The resulting strbuf rightfully has sb->len set to "0",
but is missing the NUL terminator in the first byte.

Most call-sites are fine with this. They see the EOF and
don't bother looking at the strbuf. Or they notice that
sb->len is empty, and don't look at the contents. But
there's at least one case that does neither, and relies on
parsing the resulting (possibly zero-length) string:
fast-import. You can see this in action with the new test
(though we probably only notice failure there when run with
--valgrind or ASAN).

We can fix this by unconditionally resetting the strbuf when
we have a buffer after getdelim(). That fixes case 2 above.
Case 1 is probably already fine in practice, but it does not
hurt for us to re-assert our invariants (especially because
we are relying on whatever getdelim() happens to do, which
may vary from platform to platform). Our fix covers that
case, too.

Signed-off-by: Jeff King <peff@peff.net>
---
Not a big rush for 2.8.0-rc, as the bug is in v2.5.0, and I doubt
there's an easy trigger besides fast-import. But it might be harmless
enough to squeeze in.

 strbuf.c               | 8 +++++++-
 t/t9300-fast-import.sh | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index f60e2ee..2c08dbb 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -481,9 +481,15 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	if (errno == ENOMEM)
 		die("Out of memory, getdelim failed");
 
-	/* Restore slopbuf that we moved out of the way before */
+	/*
+	 * Restore strbuf invariants; if getdelim left us with a NULL pointer,
+	 * we can just re-init, but otherwise we should make sure that our
+	 * length is empty, and that the result is NUL-terminated.
+	 */
 	if (!sb->buf)
 		strbuf_init(sb, 0);
+	else
+		strbuf_reset(sb);
 	return EOF;
 }
 #else
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 4c5f3c9..25bb60b 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -55,6 +55,10 @@ test_expect_success 'empty stream succeeds' '
 	git fast-import </dev/null
 '
 
+test_expect_success 'truncated stream complains' '
+	echo "tag foo" | test_must_fail git fast-import
+'
+
 test_expect_success 'A: create pack from stdin' '
 	test_tick &&
 	cat >input <<-INPUT_END &&
-- 
2.8.0.rc1.318.g2193183

Re: [PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Not a big rush for 2.8.0-rc, as the bug is in v2.5.0, and I doubt
> there's an easy trigger besides fast-import. But it might be harmless
> enough to squeeze in.

Was it found by a real-world debugging session, or by a code
inspection?  I'd be really impressed if it were the latter ;-)

Thanks, will queue.

>  strbuf.c               | 8 +++++++-
>  t/t9300-fast-import.sh | 4 ++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index f60e2ee..2c08dbb 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -481,9 +481,15 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
>  	if (errno == ENOMEM)
>  		die("Out of memory, getdelim failed");
>  
> -	/* Restore slopbuf that we moved out of the way before */
> +	/*
> +	 * Restore strbuf invariants; if getdelim left us with a NULL pointer,
> +	 * we can just re-init, but otherwise we should make sure that our
> +	 * length is empty, and that the result is NUL-terminated.
> +	 */
>  	if (!sb->buf)
>  		strbuf_init(sb, 0);
> +	else
> +		strbuf_reset(sb);
>  	return EOF;
>  }
>  #else
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 4c5f3c9..25bb60b 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -55,6 +55,10 @@ test_expect_success 'empty stream succeeds' '
>  	git fast-import </dev/null
>  '
>  
> +test_expect_success 'truncated stream complains' '
> +	echo "tag foo" | test_must_fail git fast-import
> +'
> +
>  test_expect_success 'A: create pack from stdin' '
>  	test_tick &&
>  	cat >input <<-INPUT_END &&

Re: [PATCH] strbuf_getwholeline: NUL-terminate getdelim buffer on error

[Jeff King]

On Sat, Mar 05, 2016 at 11:33:08AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Not a big rush for 2.8.0-rc, as the bug is in v2.5.0, and I doubt
> > there's an easy trigger besides fast-import. But it might be harmless
> > enough to squeeze in.
> 
> Was it found by a real-world debugging session, or by a code
> inspection?  I'd be really impressed if it were the latter ;-)

Sorry to disappoint, but it was the former. :)

I found (and am fixing) a _different_ bug with:

  {
	echo "tag foo"
	echo "from HEAD:./bar"
  } | git fast-import

and I wondered if fast-import would take:

  echo "tag foo from HEAD:./bar"

on one line.  The answer is no, btw, but it also tickles this bug (it is
expecting "from ..." on the next line, and handles EOF by detecting the
empty string).

-Peff