On Mon, Jul 18 2022, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
> [...]
> It is unfortunate that the lifetime of the 'entries' array is less
> clear. To make this simpler, I added a few things to try and prevent an
> accidental reference:
>
> 1. Using FREE_AND_NULL() we will at least get a segfault from reading a
> NULL pointer instead of a use-after-free.
>
> 2. 'entries_nr' is also set to zero to make any loop that would iterate
> over the entries be trivial.
>
> 3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does
> not get another reference later. This requires adding a local copy
> of 'pack_order' giving us a reference that we can use later in the
> method.
>
> 4. Add significant comments in write_midx_bitmap() and
> write_midx_internal() to add warnings for future authors who might
> accidentally add references to this cleared memory.
> [...]
> + /*
> + * Remove the ctx.entries to reduce memory pressure.
> + * Nullify 'ctx' to help avoid adding new references to ctx->entries.
> + */
> + FREE_AND_NULL(ctx->entries);
> + ctx->entries_nr = 0;
> + pack_order = ctx->pack_order;
> + ctx = NULL;
After this change this is a ~70 line function, but only 3 lines at the
top actually use ctx for anything:
/* the bug check for ctx.nr... */
prepare_midx_packing_data(&pdata, ctx);
commits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);
Did you consider just splitting it up so that that there's a "prepare
write" function? Then you don't need to worry about the scoping of ctx.
I'd think that would be better, then you also wouldn't need to implement
your own free-ing, nothing after this seems to use ctx->entries_nr (but
I just skimmed it), so it could just fall through to the free() at the
end of write_midx_internal() (the only caller), couldn't it?
