On Thu, Feb 16, 2023 at 10:01:31AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> 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:
signature.asc
Description: PGP signature
