Re: [PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Jeff King <peff@peff.net>]

On Thu, Aug 28, 2014 at 04:46:49PM +0200, Johannes Schindelin wrote:

> So far, we assumed that the buffer is NUL terminated, but this is not
> a safe assumption, now that we opened the fsck_object() API to pass a
> buffer directly.
> 
> So let's make sure that there is at least an empty line in the buffer.
> That way, our checks would fail if the empty line was encountered
> prematurely, and consequently we can get away with the current string
> comparisons even with non-NUL-terminated buffers are passed to
> fsck_object().

Hmm. So having looked through all of the code, I _think_ this is enough
because all of the parsing:

  - is left-to-right, and stops when it sees something unexpected, which
    would include a double newline

  - uses strcspn for the string-dependent calls, and each call has "\n"
    in its list of stop-characters.

But it seems kind of flaky and easy for later code to get wrong.

Would it be that hard to just allocate an extra NUL at the end of
objects we inflate in index-pack? All of the regular object code paths
provide the extra NUL-termination, and many parts of git depend on that.
I do not think we call into any other reusable code paths besides fsck()
from index-pack, but if we were to add the NUL, we would future-proof
against these "weird" object buffers getting passed elsewhere.

-Peff