Re: [PATCH] delta-islands: fix segfault when freeing island marks

From: Eric Wong <e@80x24.org>
Date: Feb 16, 2023, 12:08 UTC
Message-ID: <20230216120807.M50492@dcvr>
In-Reply-To: <61e490595b80b34c55fd640e093e021ff6fa9591.1676542973.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> wrote:

Show 12 quoted lines

> In 647982bb71 (delta-islands: free island_marks and bitmaps, 2023-02-03)
> we have introduced logic to free `island_marks` in order to reduce heap
> memory usage in git-pack-objects(1). This commit is causing segfaults in
> the case where this Git command does not load delta islands at all, e.g.
> when reading object IDs from standard input. One such crash can be hit
> when using repacking multi-pack-indices with delta islands enabled.
> 
> The root cause of this bug is that we unconditionally dereference the
> `island_marks` variable even in the case where it is a `NULL` pointer,
> which is fixed by making it conditional. Note that we still leave the
> logic in place to set the pointer to `-1` to detect use-after-free bugs
> even when there are no loaded island marks at all.

Oops, my fault :x  Thanks for this fix.

I think eliminating global flags like use_delta_islands and just allocating `struct delta_islands_foo *' in builtin/pack-objects.c would make it harder for people unfamiliar with the code to avoid bugs like this.

Tested-by: Eric Wong <e@80x24.org>

Show 7 quoted lines

> An easy way to reproduce the segfault is:
> 
>     $ git pack-objects .git/objects/pack/pack --delta-islands </dev/null
> 
> I didn't add a test for git-pack-objects(1) directly though, mostly
> because I didn't find any location to put it. I'm happy to do so though
> in case we want that.

*shrug* I'm often unsure where to add tests, too. Maybe just append it to the end of t/t5300-pack-object.sh ?