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

Re: [PATCH 4/4] midx: report checksum mismatches during 'verify'

From
Taylor Blau <me@ttaylorr.com>
Date
Nov 16, 2021, 21:10 UTC
Message-ID
<YZQeMjjtxS2LU2Tk@nand.local>
In-Reply-To
<YYzq0uBr+uoVvkbC@coredump.intra.peff.net>
On Thu, Nov 11, 2021 at 05:05:06AM -0500, Jeff King wrote:
Show 31 quoted lines
> > Since SHA1 is essentially random, there's a 1:256 chance of that
> > happening, assuming that the file's content is random.
>
> The most exact fix here would be to read the final byte, increment it
> mod-256, and write it back, which would ensure it was always changed.
> But that's a bit annoying to do in shell. Perhaps just corrupting more
> bytes is a good solution? The patch below should reduce your changes to
> 1 in 2^80.
>
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 3f69e43178..a612e44547 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -482,8 +482,10 @@ test_expect_success 'corrupt MIDX is not reused' '
>  '
>
>  test_expect_success 'verify incorrect checksum' '
> -	pos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&
> -	corrupt_midx_and_verify $pos "\377" $objdir "incorrect checksum"
> +	pos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&
> +	corrupt_midx_and_verify $pos \
> +		"\377\377\377\377\377\377\377\377\377\377" \
> +		$objdir "incorrect checksum"
>  '
>
>  test_expect_success 'repack progress off for redirected stderr' '
>
> There are other variants, of course. Just appending a single byte to the
> file is enough to give you a high probability of failing the checksum
> (since it shifts it all by one byte, making it essentially random), but
> the corrupt_midx() helper doesn't support that.
Thanks SZEDER for the report, and thanks Peff for the fix :).

I agree that it's annoyingly cumbersome to write back the last byte incremented mod-256. So I'm content to just make it astronomically unlikely to run into a collision in practice. (As a matter of fact, I'm surprised that the current implementation hasn't produced failures for us more often).

Peff: do you want me to turn this into a patch or were you planning on
it?

Thanks, Taylor

Previous: Jeff KingNext: Jeff King
Message 13 of 16 in “midx: verify MIDX checksum before reusing”
  1. 0/4 midx: verify MIDX checksum before reusingTaylor Blau, Jun 23, 2021
  2. 1/4 csum-file: introduce checksum_valid()Taylor Blau, Jun 23, 2021
  3. Jeff KingJun 24, 2021
  4. 2/4 commit-graph: rewrite to use checksum_valid()Taylor Blau, Jun 23, 2021
  5. Jeff KingJun 24, 2021
  6. 3/4 midx: don't reuse corrupt MIDXs when writingTaylor Blau, Jun 23, 2021
  7. Jeff KingJun 24, 2021
  8. 4/4 midx: report checksum mismatches during 'verify'Taylor Blau, Jun 23, 2021
  9. Bagas SanjayaJun 24, 2021
  10. Jeff KingJun 24, 2021
  11. SZEDER GáborNov 10, 2021
  12. Jeff KingNov 11, 2021
  13. Taylor BlauNov 16, 2021
  14. t5319: corrupt more bytes of the midx checksumJeff King, Nov 16, 2021
  15. Taylor BlauNov 16, 2021
  16. Derrick StoleeNov 16, 2021

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.