Re: [PATCH 04/20] commit-graph: check size of oid fanout chunk
From: Taylor Blau <hidden>
Date: 2023-10-11 01:24:48
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, Oct 10, 2023 at 08:08:05PM -0400, Taylor Blau wrote:
Do you think it would be worth changing pair_chunk() to take an expected
size_t and handle this generically? I.e. have a version of
chunk-format::pair_chunk_fn() that looks something like:
static int pair_chunk_fn(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
const unsigned char **p = data;
if (chunk_size != data->size)
return -1;
*p = chunk_start;
return 0;
}
and then our call here would be:
if (pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,
(const unsigned char **)&graph->chunk_oid_fanout,
256 * sizeof(uint32_t)) < 0)
return error("commit-graph oid fanout chunk is wrong size");I took a brief stab at implementing this tonight and came up with this, which applies on top of this patch. Looking through the rest of the series briefly[^1], I think having something like this would be useful:
--- 8< ---
diff --git a/chunk-format.c b/chunk-format.c
index 8d910e23f6..38b0f847be 100644
--- a/chunk-format.c
+++ b/chunk-format.c@@ -157,6 +157,8 @@ int read_table_of_contents(struct chunkfile *cf, struct pair_chunk_data { const unsigned char **p; size_t *size; + + size_t expected_size; }; static int pair_chunk_fn(const unsigned char *chunk_start,
@@ -169,6 +171,20 @@ static int pair_chunk_fn(const unsigned char *chunk_start, return 0; } +static int pair_chunk_expect_fn(const unsigned char *chunk_start, + size_t chunk_size, + void *data) +{ + struct pair_chunk_data *pcd = data; + if (pcd->expected_size != chunk_size) + return error(_("mismatched chunk size, got: %"PRIuMAX", wanted:" + " %"PRIuMAX), + (uintmax_t)chunk_size, + (uintmax_t)pcd->expected_size); + *pcd->p = chunk_start; + return 0; +} + int pair_chunk(struct chunkfile *cf, uint32_t chunk_id, const unsigned char **p,
@@ -178,6 +194,14 @@ int pair_chunk(struct chunkfile *cf, return read_chunk(cf, chunk_id, pair_chunk_fn, &pcd); } +int pair_chunk_expect(struct chunkfile *cf, + uint32_t chunk_id, const unsigned char **p, + size_t expected_size) +{ + struct pair_chunk_data pcd = { .p = p, .expected_size = expected_size }; + return read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd); +} + int pair_chunk_unsafe(struct chunkfile *cf, uint32_t chunk_id, const unsigned char **p)
diff --git a/chunk-format.h b/chunk-format.h
index 8dce7967f4..778f81f555 100644
--- a/chunk-format.h
+++ b/chunk-format.h@@ -53,6 +53,10 @@ int pair_chunk(struct chunkfile *cf, const unsigned char **p, size_t *size); +int pair_chunk_expect(struct chunkfile *cf, + uint32_t chunk_id, const unsigned char **p, + size_t expected_size); + /* * Unsafe version of pair_chunk; it does not return the size, * meaning that the caller cannot possibly be careful about
diff --git a/commit-graph.c b/commit-graph.c
index 9b3b01da61..ed85161fdb 100644
--- a/commit-graph.c
+++ b/commit-graph.c@@ -305,16 +305,6 @@ static int verify_commit_graph_lite(struct commit_graph *g) return 0; } -static int graph_read_oid_fanout(const unsigned char *chunk_start, - size_t chunk_size, void *data) -{ - struct commit_graph *g = data; - if (chunk_size != 256 * sizeof(uint32_t)) - return error("commit-graph oid fanout chunk is wrong size"); - g->chunk_oid_fanout = (const uint32_t *)chunk_start; - return 0; -} - static int graph_read_oid_lookup(const unsigned char *chunk_start, size_t chunk_size, void *data) {
@@ -404,7 +394,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s, GRAPH_HEADER_SIZE, graph->num_chunks)) goto free_and_return; - read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph); + if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT, + (const unsigned char **)&graph->chunk_oid_fanout, + 256 * sizeof(uint32_t)) < 0) + error(_("commit-graph oid fanout chunk is wrong size")); read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph); pair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data); pair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d25bea3ec5..467b5f5e9c 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh@@ -841,6 +841,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' ' # otherwise we hit an earlier check check_corrupt_chunk OIDF clear $(printf "000000%02x" $(test_seq 250)) && cat >expect.err <<-\EOF && + error: mismatched chunk size, got: 1000, wanted: 1024 error: commit-graph oid fanout chunk is wrong size error: commit-graph is missing the OID Fanout chunk EOF --- >8 ---
Or, quite honestly, having the "expected_size" parameter be required in the safe version of `pair_chunk()`. Thanks, Taylor [^1]: A non-brief review is on my to-do list for tomorrow.