Re: [PATCH v4 3/8] finalize_object_file(): implement collision check

From: Jeff King <peff@peff.net>
Date: Sep 24, 2024, 22:20 UTC
Message-ID: <20240924222039.GB1148242@coredump.intra.peff.net>
In-Reply-To: <ZvM2Lkb0/LPrqizO@nand.local>

On Tue, Sep 24, 2024 at 05:59:10PM -0400, Taylor Blau wrote:

Show 9 quoted lines

> > So the argument I'd make here is more like: this is the wrong place to
> > do it.
> 
> I think that is reasonable, and I agree with your reasoning here. I'm
> happy to reword the commit message if you think doing so would be
> useful, or drop the paragraph entirely if you think it's just confusing.
> 
> Let me know what you think, I'm happy to do whatever here, reroll or not
> :-).

I'm content to let this live in the list archive, but it sounds like Junio had the same reaction, so it may be worth trying to rework the commit message a bit.

Show 11 quoted lines

> >     In general, the use of unpack-objects versus index-pack is up to the
> >     attacker (based on pack size). So I think it would be nice if
> >     unpack-objects did the collision check. Even if the attacker beats
> >     you to writing the object, it would be nice to see "holy crap, there
> >     was a collision" instead of just silently throwing your pushed
> >     object away.
> 
> Right, I agree that unpack-objects definitely should do the collision
> check here. And indeed, that is the case, since that code (which all is
> directly implemented in the builtin) uses the regular
> collision-detecting SHA-1 implementation.

It uses sha1dc, but what I mean by "collision check" is an additional belt-and-suspenders check. That even once we see an object which hashes to a particular sha1 which we already have, we'll do the byte-for-byte comparison. See index-pack's "check_collison".

And that's what I think should be added to unpack-objects (but again, this is all orthogonal to your series).

Show 9 quoted lines

> I thought about this when writing it, and came to the conclusion that
> the checks we have for "are we in something that looks like a loose
> object shard?" are sane. That's only because we won't bother reading a
> pack in $GIT_DIR/objects/pack/xx/yy.pack, since we do not read packs
> recursively from the pack sub-directory.
> 
> So I think that the diff you posted below isn't hurting anything, and
> the implementation looks correct to me, but I also don't think it's
> helping anything either as a consequence of the above.

Right, it's purely about future-proofing against a hypothetical change where we'd be more inclusive (both in what we read, but also in what we'd ourselves write!).

-Peff