| Message ID | 20231109070310.GA2697602@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | some more chunk-file bounds-checks fixes | expand |
On Thu, Nov 09, 2023 at 02:03:10AM -0500, Jeff King wrote: > This is a follow-up to the series from: > > https://lore.kernel.org/git/20231009205544.GA3281950@coredump.intra.peff.net/ > > which was merged to master as jk/chunk-bounds. There were a few issues > left open by that series and its review: > > 1. the midx code didn't check fanout ordering > > 2. whether we needed to sprinkle some more st_mult() on it > > 3. improving some of the error messages (translations, some > consistency, maybe more details) > > 4. possible refactoring with a pair_chunk_expect() API (Taylor posted > a series in that direction, which is currently in limbo) I read this series thoroughly and was very pleased with the result. Thanks for patching these up. I think that I am still of the mind that it would be useful to have some kind of pair_chunk_expect() function, so I'll try and rebase/rewrite my patches on top of your new ones here. In the meantime, this series LGTM. Thanks, Taylor
This is a follow-up to the series from: https://lore.kernel.org/git/20231009205544.GA3281950@coredump.intra.peff.net/ which was merged to master as jk/chunk-bounds. There were a few issues left open by that series and its review: 1. the midx code didn't check fanout ordering 2. whether we needed to sprinkle some more st_mult() on it 3. improving some of the error messages (translations, some consistency, maybe more details) 4. possible refactoring with a pair_chunk_expect() API (Taylor posted a series in that direction, which is currently in limbo) The patches here fix the remaining correctness issues (points 1 and 2, along with a few other small issues I found). There's some improvement on 3, but I stopped short of adding lots more details. Partially because the series was already getting big, and partially because going further may depend on what we do with 4. With regards to the pair_chunk_expect() thing, this series is incompatible (textually, but not conceptually) with what Taylor posted earlier, just because I'm moving some checks into the chunk-reader callbacks. Because it's fixing user-visible bugs, I think we'd want to do this first, and then (possibly) rebase Taylor's series on top. But I also think some of the things I noticed around overflow (especially patches 1 and 6) may inform how we'd want the pair_chunk_expect() API to look. This is a continuation of the jk/chunk-bounds topic, which is new in the v2.43 cycle. But it should be OK to leave this until after the release. Nothing here is fixing a regression in the 2.43 release candidates; it's just a few bits that were incomplete. That said, I did try to float the correctness bits to the first two patches just in case. ;) [1/9]: commit-graph: handle overflow in chunk_size checks [2/9]: midx: check consistency of fanout table [3/9]: commit-graph: drop redundant call to "lite" verification [4/9]: commit-graph: clarify missing-chunk error messages [5/9]: commit-graph: abort as soon as we see a bogus chunk [6/9]: commit-graph: use fanout value for graph size [7/9]: commit-graph: check order while reading fanout chunk [8/9]: commit-graph: drop verify_commit_graph_lite() [9/9]: commit-graph: mark chunk error messages for translation commit-graph.c | 94 +++++++++++++------------------------ midx.c | 20 ++++---- t/t5318-commit-graph.sh | 16 ++++--- t/t5319-multi-pack-index.sh | 14 ++++++ 4 files changed, 67 insertions(+), 77 deletions(-) -Peff