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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2023, 18:01 UTC
Message-ID: <xmqqbkltqykk.fsf@gitster.g>
In-Reply-To: <61e490595b80b34c55fd640e093e021ff6fa9591.1676542973.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

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.

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.