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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 25, 2024, 18:06 UTC
Message-ID: <ZvRRCb0+82p/IeI3@nand.local>
In-Reply-To: <20240924222039.GB1148242@coredump.intra.peff.net>

On Tue, Sep 24, 2024 at 06:20:39PM -0400, Jeff King wrote:

Show 15 quoted lines

> On Tue, Sep 24, 2024 at 05:59:10PM -0400, Taylor Blau wrote:
>
> > > 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.

Here's the relevant portion of my range-diff that has the new wording (which is more or less equivalent to your "this isn't the right place to do it, and we're not fundamentally changing anything from a security perspective here" argument). Let me know what you think:

--- 8< ---
3:  ed9eeef851 ! 3:  41d38352a4 finalize_object_file(): implement collision check
    @@ Commit message
             object name, so checking for collisions at the content level doesn't
             add anything.

    -        This is why we do not bother to check the inflated object contents
    -        for collisions either, since either (a) the object contents have the
    -        fingerprint of a SHA-1 collision, in which case the collision
    -        detecting SHA-1 implementation used to hash the contents to give us
    -        a path would have already rejected it, or (b) the contents are part
    -        of a colliding pair which does not bear the same fingerprints of
    -        known collision attacks, in which case we would not have caught it
    -        anyway.
    +        Adding a content-level collision check would have to happen at a
    +        higher level than in finalize_object_file(), since (avoiding race
    +        conditions) writing an object loose which already exists in the
    +        repository will prevent us from even reaching finalize_object_file()
    +        via the object freshening code.
    +
    +        There is a collision check in index-pack via its `check_collision()`
    +        function, but there isn't an analogous function in unpack-objects,
    +        which just feeds the result to write_object_file().

             So skipping the collision check here does not change for better or
             worse the hardness of loose object writes.
    @@ Commit message

         Co-authored-by: Jeff King <peff@peff.net>
         Signed-off-by: Jeff King <peff@peff.net>
    +    Helped-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>

      ## object-file.c ##
--- >8 ---

Thanks, Taylor