Skip to content

Commit 4f85432

Browse files
ttaylorrgitster
authored andcommitted
midx-write.c: introduce midx_pack_perm() helper
The `ctx->pack_perm` array can be considered as a permutation between the original `pack_int_id` of some given pack to its position in the `ctx->info` array containing all packs. Today we can always index into this array with any known `pack_int_id`, since there is never a `pack_int_id` which is greater than or equal to the value `ctx->nr`. That is not necessarily the case with MIDX compaction. For example, suppose we have a MIDX chain with three layers, each containing three packs. The base of the MIDX chain will have packs with IDs 0, 1, and 2, the next layer 3, 4, and 5, and so on. If we are compacting the topmost two layers, we'll have input `pack_int_id` values between [3, 8], but `ctx->nr` will only be 6. In that example, if we want to know where the pack whose original `pack_int_id` value was, say, 7, we would compute `ctx->pack_perm[7]`, leading to an uninitialized read, since there are only 6 entries allocated in that array. To address this, there are a couple of options: - We could allocate enough entries in `ctx->pack_perm` to accommodate the largest `orig_pack_int_id` value. - Or, we could internally shift the input values by the number of packs in the base layer of the lower end of the MIDX compaction range. This patch prepare us to take the latter approach, since it does not allocate more memory than strictly necessary. (In our above example, the base of the lower end of the compaction range is the first MIDX layer (having three packs), so we would end up indexing `ctx->pack_perm[7-3]`, which is a valid read.) Note that this patch does not actually implement that approach yet, but merely performs a behavior-preserving refactoring which will make the change easier to carry out in the future. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent b2ec8e9 commit 4f85432

File tree

1 file changed

+12
-6
lines changed

1 file changed

+12
-6
lines changed

midx-write.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,12 @@ struct write_midx_context {
119119
struct odb_source *source;
120120
};
121121

122+
static uint32_t midx_pack_perm(struct write_midx_context *ctx,
123+
uint32_t orig_pack_int_id)
124+
{
125+
return ctx->pack_perm[orig_pack_int_id];
126+
}
127+
122128
static int should_include_pack(const struct write_midx_context *ctx,
123129
const char *file_name)
124130
{
@@ -521,12 +527,12 @@ static int write_midx_object_offsets(struct hashfile *f,
521527
for (i = 0; i < ctx->entries_nr; i++) {
522528
struct pack_midx_entry *obj = list++;
523529

524-
if (ctx->pack_perm[obj->pack_int_id] == PACK_EXPIRED)
530+
if (midx_pack_perm(ctx, obj->pack_int_id) == PACK_EXPIRED)
525531
BUG("object %s is in an expired pack with int-id %d",
526532
oid_to_hex(&obj->oid),
527533
obj->pack_int_id);
528534

529-
hashwrite_be32(f, ctx->pack_perm[obj->pack_int_id]);
535+
hashwrite_be32(f, midx_pack_perm(ctx, obj->pack_int_id));
530536

531537
if (ctx->large_offsets_needed && obj->offset >> 31)
532538
hashwrite_be32(f, MIDX_LARGE_OFFSET_NEEDED | nr_large_offset++);
@@ -627,7 +633,7 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)
627633
for (i = 0; i < ctx->entries_nr; i++) {
628634
struct pack_midx_entry *e = &ctx->entries[i];
629635
data[i].nr = i;
630-
data[i].pack = ctx->pack_perm[e->pack_int_id];
636+
data[i].pack = midx_pack_perm(ctx, e->pack_int_id);
631637
if (!e->preferred)
632638
data[i].pack |= (1U << 31);
633639
data[i].offset = e->offset;
@@ -637,7 +643,7 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)
637643

638644
for (i = 0; i < ctx->entries_nr; i++) {
639645
struct pack_midx_entry *e = &ctx->entries[data[i].nr];
640-
struct pack_info *pack = &ctx->info[ctx->pack_perm[e->pack_int_id]];
646+
struct pack_info *pack = &ctx->info[midx_pack_perm(ctx, e->pack_int_id)];
641647
if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)
642648
pack->bitmap_pos = i + base_objects;
643649
pack->bitmap_nr++;
@@ -698,7 +704,7 @@ static void prepare_midx_packing_data(struct packing_data *pdata,
698704
struct object_entry *to = packlist_alloc(pdata, &from->oid);
699705

700706
oe_set_in_pack(pdata, to,
701-
ctx->info[ctx->pack_perm[from->pack_int_id]].p);
707+
ctx->info[midx_pack_perm(ctx, from->pack_int_id)].p);
702708
}
703709

704710
trace2_region_leave("midx", "prepare_midx_packing_data", ctx->repo);
@@ -1384,7 +1390,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
13841390
sizeof(*ctx.info),
13851391
idx_or_pack_name_cmp);
13861392
if (preferred) {
1387-
uint32_t perm = ctx.pack_perm[preferred->orig_pack_int_id];
1393+
uint32_t perm = midx_pack_perm(&ctx, preferred->orig_pack_int_id);
13881394
if (perm == PACK_EXPIRED)
13891395
warning(_("preferred pack '%s' is expired"),
13901396
opts->preferred_pack_name);

0 commit comments

Comments
 (0)