Re: [PATCH 2/6] Accept object data in the fsck_object() function

[Jeff King <peff@peff.net>]

On Thu, Aug 28, 2014 at 01:47:52PM -0700, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > When fsck'ing an incoming pack, we need to fsck objects that cannot be
> > read via read_sha1_file() because they are not local yet (and might even
> > be rejected if transfer.fsckobjects is set to 'true').
> >
> > For commits, there is a hack in place: we basically cache commit
> > objects' buffers anyway, but the same is not true, say, for tag objects.
> >
> > By refactoring fsck_object() to take the object buffer and size as
> > optional arguments -- optional, because we still fall back to the
> > previous method to look at the cached commit objects if the caller
> > passes NULL -- we prepare the machinery for the upcoming handling of tag
> > objects.
> >
> > The assumption that such buffers are inherently NUL terminated is now
> > wrong, of course, hence we pass the size of the buffer so that we can
> > add a sanity check later, to prevent running past the end of the buffer.
> 
> A nice side effect may be that we can now check (and perhaps warn) a
> commit buffer with a NUL inside, perhaps?   I am not suggesting to
> add such a check to this series, but mentioning the possibilty here
> may have a merit.

I think that is a good check to add at some point. I demonstrated quite
a while ago that you can get up to some mischief by "hiding" bytes after
a NUL in the commit message. If we ever see feasible collision attacks
against sha1, this is going to be an obvious vector for hiding random
bytes to achieve the collisions.

The downside is that git's data model in theory promises to store
arbitrary bytes in people's commit messages. IMHO we have gotten far
enough from that in practice that I do not think anybody is seriously
doing it (you could not really use most of the history inspection tools
in a reasonable way; you would have to write a parallel set of tools
that never shows log messages, and then extracts and does something
separate with the commit content).

-Peff