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

From: Jeff King <peff@peff.net>
Date: Dec 8, 2014, 11:47 UTC
Message-ID: <20141208114740.GA16658@peff.net>
In-Reply-To: <alpine.DEB.1.00.1412081232500.13845@s15462909.onlinehome-server.info>

On Mon, Dec 08, 2014 at 12:35:27PM +0100, Johannes Schindelin wrote:

Show 13 quoted lines

> On Mon, 8 Dec 2014, Duy Nguyen wrote:
> 
> > On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:
> > > 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).
> > 
> > I think I'm missing a "but.." here.
> 
> The "but..."s I have are:
> 
> 1) we potentially waste space, and

I think this can be ignored. It's 1 byte per object, and only while we keep the object in RAM. Also, we already do it for buffers read from read_sha1_file, so when you run "git log" every commit buffer we keep in RAM is already doing this (and has been since basically day one).

> 2) I would like to make really certain, preferably with static analysis,
>    that fsck_object() only receives buffers that are NUL terminated, and
>    that no call path is missed.

I know this is not as good as a real static analysis, but I was concerned about this exact thing about a year ago (I think in relation to commit parsing for pretty-printing) and traced all of the paths through which you can get an object; they all end up in the same few code paths that all xmallocz: unpack_sha1_file for loose objects, unpack_compressed_entry for pack bases, and patch_delta for deltas.

Index-pack and unpack-objects are the odd men out here because they are processing objects that are not actually in the repository yet. I think the spots Duy pointed out probably cover index-pack. It looks like builtin/unpack-objects.c:get_data needs the same treatment.

I know that Duy mentioned a while ago killing off unpack-objects and rolling its functionality into index-pack. That would be a very nice thing to do if somebody can find the time. There's a fair bit of duplication, and index-pack receives a lot more attention (so it's faster, and probably more robust against weird incoming packs).

-Peff