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

Re: [PATCH 5/9] commit-graph: abort as soon as we see a bogus chunk

From
Taylor Blau <me@ttaylorr.com>
Date
Nov 9, 2023, 21:18 UTC
Message-ID
<ZU1L8qdIjz/OIAfR@nand.local>
In-Reply-To
<20231109071711.GE2698043@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:17:11AM -0500, Jeff King wrote:
Show 24 quoted lines
> The code to read commit-graph files tries to read all of the required
> chunks, but doesn't abort if we can't find one (or if it's corrupted).
> It's only at the end of reading the file that we then do some sanity
> checks for NULL entries. But it's preferable to detect the errors and
> bail immediately, for a few reasons:
>
>   1. It's less error-prone. It's easy in the reader functions to flag an
>      error but still end up setting some struct fields (an error I in
>      fact made while working on this patch series).
>
>   2. It's safer. Since verifying some chunks depends on the values of
>      other chunks, we may be depending on not-yet-verified data. I don't
>      know offhand of any case where this can cause problems, but it's
>      one less subtle thing to worry about in the reader code.
>
>   3. It prevents the user from seeing nonsense errors. If we're missing
>      an OIDL chunk, then g->num_commits will be zero. And so we may
>      complain that the size of our CDAT chunk (which should have a
>      fixed-size record for each commit) is wrong unless it's also zero.
>      But that's misleading; the problem is the missing OIDL chunk; the
>      CDAT one might be fine!
>
> So let's just check the return value from read_chunk(). This is exactly
> how the midx chunk-reading code does it.

All very well explained. I hit that same snag as you did when I was working on the few patches I proposed we put on top of your earlier chunk-format hardening series.

I'm glad to see this getting cleaned up, and I'm very happy with the post-image of this patch.

Thanks, Taylor

Previous: Jeff KingNext: Jeff King
Message 9 of 39 in “some more chunk-file bounds-checks fixes”
  1. 0/9 some more chunk-file bounds-checks fixesJeff King, Nov 9, 2023
  2. 1/9 commit-graph: handle overflow in chunk_size checksJeff King, Nov 9, 2023
  3. Taylor BlauNov 9, 2023
  4. Jeff KingNov 9, 2023
  5. 2/9 midx: check consistency of fanout tableJeff King, Nov 9, 2023
  6. 3/9 commit-graph: drop redundant call to "lite" verificationJeff King, Nov 9, 2023
  7. 4/9 commit-graph: clarify missing-chunk error messagesJeff King, Nov 9, 2023
  8. 5/9 commit-graph: abort as soon as we see a bogus chunkJeff King, Nov 9, 2023
  9. Taylor BlauNov 9, 2023
  10. 6/9 commit-graph: use fanout value for graph sizeJeff King, Nov 9, 2023
  11. Taylor BlauNov 9, 2023
  12. Jeff KingNov 9, 2023
  13. Taylor BlauNov 9, 2023
  14. Jeff KingNov 10, 2023
  15. 7/9 commit-graph: check order while reading fanout chunkJeff King, Nov 9, 2023
  16. 8/9 commit-graph: drop verify_commit_graph_lite()Jeff King, Nov 9, 2023
  17. 9/9 commit-graph: mark chunk error messages for translationJeff King, Nov 9, 2023
  18. Taylor BlauNov 9, 2023
  19. 0/7 chunk-format: introduce `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  20. 1/7 chunk-format: introduce `pair_chunk_expect()` helperTaylor Blau, Nov 9, 2023
  21. Junio C HamanoNov 10, 2023
  22. Taylor BlauNov 10, 2023
  23. Jeff KingNov 10, 2023
  24. Junio C HamanoNov 10, 2023
  25. Junio C HamanoNov 10, 2023
  26. Jeff KingNov 10, 2023
  27. Jeff KingNov 10, 2023
  28. Jeff KingNov 10, 2023
  29. Linus ArverJan 15, 2024
  30. Linus ArverJan 15, 2024
  31. Jeff KingJan 16, 2024
  32. Linus ArverJan 18, 2024
  33. 2/7 commit-graph: read `OIDL` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  34. Jeff KingNov 10, 2023
  35. 3/7 commit-graph: read `CDAT` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  36. 4/7 commit-graph: read `GDAT` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  37. 5/7 commit-graph: read `BIDX` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  38. 6/7 midx: read `OIDL` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  39. 7/7 midx: read `OOFF` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023

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.