diff mbox series

[v2,4/4] builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects

Message ID 3fc3a95517558005b940b65d0e5357de50f81d98.1653418457.git.me@ttaylorr.com (mailing list archive)
State Accepted
Commit 4090511e408a37091e7812bc1828a763a035e0a2
Headers show
Series pack-objects: fix a pair of MIDX bitmap-related races | expand

Commit Message

Taylor Blau May 24, 2022, 6:54 p.m. UTC
When using a multi-pack bitmap, pack-objects will try to perform its
traversal using a call to `traverse_bitmap_commit_list()`, which calls
`add_object_entry_from_bitmap()` to add each object it finds to its
packing list.

This path can cause pack-objects to add objects from packs that don't
have open pack_fds on them, by avoiding a call to `is_pack_valid()`.
This is because we only call `is_pack_valid()` on the preferred pack (in
order to do verbatim reuse via `reuse_partial_packfile_from_bitmap()`)
and not others when loading a MIDX bitmap.

In this case, `add_object_entry_from_bitmap()` will check whether it
wants each object entry by calling `want_object_in_pack()`, which will
call `want_found_object` (since its caller already supplied a
`found_pack`). In most cases (particularly without `--local`, and when
`ignored_packed_keep_on_disk` and `ignored_packed_keep_in_core` are
both "0"), we'll take the entry from the pack contained in the MIDX
bitmap, all without an open pack_fd.

When we then try to use that entry later to assemble the actual pack,
we'll be susceptible to any simultaneous writers moving that pack out of
the way (e.g., due to a concurrent repack) without having an open file
descriptor, causing races that result in errors like:

    remote: Enumerating objects: 1498802, done.
    remote: fatal: packfile ./objects/pack/pack-e57d433b5a588daa37fbe946e2b28dfaec03a93e.pack cannot be accessed
    remote: aborting due to possible repository corruption on the remote side.

This race can happen even with multi-pack bitmaps, since we may open a
MIDX bitmap that is being rewritten long before its packs are actually

Work around this by calling `is_pack_valid()` from within
`want_found_object()`, matching the behavior in
`want_object_in_pack_one()` (which has an analogous call). Most calls to
`is_pack_valid()` should be basically no-ops, since only the first call
requires us to open a file (subsequent calls realize the file is already
open, and return immediately).

Importantly, when `want_object_in_pack()` is given a non-NULL
`*found_pack`, but `want_found_object()` rejects the copy of the object
in that pack, we must reset `*found_pack` and `*found_offset` to NULL
and 0, respectively. Failing to do so could lead to other checks in
`want_object_in_pack()` (such as `want_object_in_pack_one()`) using the
same (invalid) pack as `*found_pack`, meaning that we don't call
`is_pack_valid()` because `p == *found_pack`. This can lead the caller
to believe it can use a copy of an object from an invalid pack.

An alternative approach to closing this race would have been to call
`is_pack_valid()` on _all_ packs in a multi-pack bitmap on load. This
has a couple of problems:

  - it is unnecessarily expensive in the cases where we don't actually
    need to open any packs (e.g., in `git rev-list --use-bitmap-index

  - more importantly, it means any time we would have hit this race,
    we'll avoid using bitmaps altogether, leading to significant
    slowdowns by forcing a full object traversal

Co-authored-by: Victoria Dye <vdye@github.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
 builtin/pack-objects.c | 6 ++++++
 1 file changed, 6 insertions(+)
diff mbox series


diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ffeaecd1d8..0a26de166d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1357,6 +1357,9 @@  static int want_found_object(const struct object_id *oid, int exclude,
 	if (incremental)
 		return 0;
+	if (!is_pack_valid(p))
+		return -1;
 	 * When asked to do --local (do not include an object that appears in a
 	 * pack we borrow from elsewhere) or --honor-pack-keep (do not include
@@ -1472,6 +1475,9 @@  static int want_object_in_pack(const struct object_id *oid,
 		want = want_found_object(oid, exclude, *found_pack);
 		if (want != -1)
 			return want;
+		*found_pack = NULL;
+		*found_offset = 0;
 	for (m = get_multi_pack_index(the_repository); m; m = m->next) {