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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On Thu, Feb 16, 2023 at 10:01:31AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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.
> 
> There still are unprotected uses of island_marks in delta-islands.c
> after this patch, but I think they are safe.
> 
>  * The callchain deduplicate_islands() -> mark_remote_island_1() ->
>    create_or_get_island_marks() assume island_marks is not NULL, and
>    the only caller of deduplicate_islands(), load_delta_islands(),
>    initializes island_marks() before calling into it.
> 
>  * set_island_marks() assumes island_marks is not NULL.  One of its
>    two callers, resolve_tree_islands() ensures island_marks is not
>    NULL before proceeding.  The other one, propagate_island_marks(),
>    also assumes island_marks is not NULL, and is called only from
>    pack-objects.c::show_commit() when use_delta_islands is set.  It
>    is not apparent if island_marks has already populated when this
>    happens, though.
> 
> I think early in the pack-objects process, prepare_pack() calls
> resolve_tree_islands() but as we have seen, it just punts when
> island_marks is NULL, and not populates.  But get_object_list()
> explicitly calls load_delta_islands() when use_delta_islands is
> set, and that happens way before prepare_pack() gets called, so we
> should be safe.
> 
> Thanks.

Agreed. I also had a look at the other usages of `island_marks` and
couldn't find any obvious way to access it without it being initialized.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]