git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: SHA1 collision in production repo?! (probably not)

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 31, 2017, 18:19 UTC
Message-ID
<xmqq4ly9z4tx.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20170331174827.zheqstwtlsqtxa6e@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> On Fri, Mar 31, 2017 at 01:45:15PM -0400, Jeff King wrote:
>
>> I suspect this may improve things, but I haven't dug deeper to see if
>> there are unwanted side effects, or if there are other spots that need
>> similar treatment.
>> 
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 43990dec7..38411f90b 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>>  	if (status && oi->typep)
>>  		*oi->typep = status;
>>  	strbuf_release(&hdrbuf);
>> -	return 0;
>> +	return status;
>>  }
>>  
>>  int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)
>
> Er, no, that's totally wrong. "status' may be holding the type. It
> should really be:
>
>   return status < 0 ? status : 0;

Sounds more like it. The only caller will say "ah, that object is not available to us---let's try packs again", which is exactly what we want to happen.

There is another bug in the codepath: the assignment to *oi->typep in the pre-context must guard against negative status value. By returning an error correctly like you do above, that bug becomes more or less irrelevant, though.

Previous: Jeff KingNext: Jeff King
Message 6 of 15 in “SHA1 collision in production repo?! (probably not)”
  1. Lars SchneiderMar 31, 2017
  2. Jeff KingMar 31, 2017
  3. Junio C HamanoMar 31, 2017
  4. Jeff KingMar 31, 2017
  5. Jeff KingMar 31, 2017
  6. Junio C HamanoMar 31, 2017
  7. Jeff KingMar 31, 2017
  8. Junio C HamanoMar 31, 2017
  9. Jeff KingApr 1, 2017
  10. 1/2 sha1_loose_object_info: return error for corrupted objectsJeff King, Apr 1, 2017
  11. Junio C HamanoApr 1, 2017
  12. 2/2 index-pack: detect local corruption in collision checkJeff King, Apr 1, 2017
  13. Junio C HamanoApr 1, 2017
  14. Lars SchneiderSep 12, 2017
  15. Jeff KingSep 12, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.