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

2 messages, 2 authors, 2023-02-20 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2023-02-16 17:36:25

Patrick Steinhardt [off-list ref] writes:
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.
---
Missing sign-off?

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

From: Patrick Steinhardt <hidden>
Date: 2023-02-20 07:38:36

On Thu, Feb 16, 2023 at 09:36:20AM -0800, Junio C Hamano wrote:
Patrick Steinhardt [off-list ref] writes:
quoted
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.
---
Missing sign-off?
Oops, right. Please feel free to add it when pulling:

    Signed-off-by: Patrick Steinhardt [off-list ref]

If you prefer I'd also be happy to send another iteration of this patch,
potentially including a direct test for git-pack-objects(1). I'm still
not sure whether it'd be worth it though given that it's implicitly
covered by the higher-level test that checks for the segfault by using
git-multi-pack-index(1).

Patrick
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help