Re: [PATCH] fsck: properly bound "invalid tag name" error message

[Jeff King <peff@peff.net>]

On Mon, Dec 08, 2014 at 12:48:12AM -0500, Jeff King wrote:

> Note that when fscking tags with "index-pack --strict", this
> is even worse. index-pack does not add a trailing
> NUL-terminator after the object, so we may actually read
> past the buffer and print uninitialized memory. Running
> t5302 with valgrind does notice the bug for that reason.

This merits an additional note (but fortunately not a patch :) ).

After writing the above, I thought for a moment that we might actually
read past the end of the buffer in some cases, but I convinced myself
otherwise. And I think Dscho and I might have even had this conversation
off-list a while ago, but I think it is worth pointing out so that
nobody else has to dig into it.

For the most part, we are fine because we parse the object
left-to-right, and barf as soon as we see something unusual (and for
this reason, fsck_commit_buffer is also fine). The two suspicious places
are:

  1. We call strchr(buffer, '\n'), which looks like it could read
     unbounded when "buffer" is not NUL-terminated. However, early in
     the function we confirm that it contains "\n\n", and we will not
     have parsed past that here. Therefore we know that we will always
     hit a newline.

  2. After finding and parsing a line whose trailing newline is marked
     by "eol", we then set "buffer = eol + 1". This would be wrong if
     eol is at the very end of the buffer (the next step would then
     start reading uninitialized memory).

     But again we are saved by the "\n\n" check. The strchr will always
     find the first, so we know that we have at least one character
     after it (and that character is a newline, which cannot be the
     start of a new header, which will cause us to stop parsing).

I do admit that I am tempted to teach index-pack to always NUL-terminate
objects in memory that we feed to fsck, just to be on the safe side. It
doesn't cost much, and could prevent a silly mistake (either in the
future, or one that I missed in my analysis). The fsck code otherwise
generally expects to get the output of read_sha1_file, which has the
safety-NUL appended.

-Peff