On Tue, Sep 24, 2024 at 06:20:39PM -0400, Jeff King wrote:
> 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@xxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
+ Helped-by: Elijah Newren <newren@xxxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
## object-file.c ##
--- >8 ---
Thanks,
Taylor
