git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: vdye@github.com, jonathantanmy@google.com, gitster@pobox.com
Subject: [PATCH v2 0/4] pack-objects: fix a pair of MIDX bitmap-related races
Date: Tue, 24 May 2022 14:54:18 -0400	[thread overview]
Message-ID: <cover.1653418457.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1652458395.git.me@ttaylorr.com>

This is a small-ish reroll of mine and Victoria's series to fix a couple of
races related to using multi-pack bitmaps in pack-objects.

The crux of both is that we call `is_pack_valid()` far too late, leaving us in a
situation where `pack-objects` committed to using objects from a specific pack
in the MIDX bitmap, without having actually opened those packs. So if those
packs are removed in the background (e.g., due to a simultaneous repack),
any ongoing clones or fetches will see this error message:

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

The first patch is mostly unchanged (except for removing an accidental
double-close()), but the second patch has itself turned into three new patches
in order to resolve the issue described in [1].

Thanks in advance for your review!

[1]: https://lore.kernel.org/git/Yn+v8mEHm2sfo4sn@nand.local/

Taylor Blau (4):
  pack-bitmap.c: check preferred pack validity when opening MIDX bitmap
  builtin/pack-objects.c: avoid redundant NULL check
  builtin/pack-objects.c: ensure included `--stdin-packs` exist
  builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects

 builtin/pack-objects.c | 43 +++++++++++++++++++++++++-----------------
 pack-bitmap.c          | 18 ++++++++++++++++--
 2 files changed, 42 insertions(+), 19 deletions(-)

Range-diff against v1:
1:  83e2ad3962 ! 1:  618e8a6166 pack-bitmap: check preferred pack validity when opening MIDX bitmap
    @@ Metadata
     Author: Taylor Blau <me@ttaylorr.com>
     
      ## Commit message ##
    -    pack-bitmap: check preferred pack validity when opening MIDX bitmap
    +    pack-bitmap.c: check preferred pack validity when opening MIDX bitmap
     
         When pack-objects adds an entry to its packing list, it marks the
         packfile and offset containing the object, which we may later use during
         verbatim reuse (c.f., `write_reused_pack_verbatim()`).
     
         If the packfile in question is deleted in the background (e.g., due to a
    -    concurrent `git repack`), we'll die() as a result of calling use_pack().
    -    4c08018204 (pack-objects: protect against disappearing packs,
    -    2011-10-14) worked around this by opening the pack ahead of time before
    -    recording it as a valid source for reuse.
    +    concurrent `git repack`), we'll die() as a result of calling use_pack(),
    +    unless we have an open file descriptor on the pack itself. 4c08018204
    +    (pack-objects: protect against disappearing packs, 2011-10-14) worked
    +    around this by opening the pack ahead of time before recording it as a
    +    valid source for reuse.
     
         4c08018204's treatment meant that we could tolerate disappearing packs,
    -    since it ensures we always have an open file descriptor any pack that we
    -    mark as a valid source for reuse. This tightens the race to only happen
    -    when we need to close an open pack's file descriptor (c.f., the caller
    -    of `packfile.c::get_max_fd_limit()`) _and_ that pack was deleted, in
    -    which case we'll complain that a pack could not be accessed and die().
    +    since it ensures we always have an open file descriptor on any pack that
    +    we mark as a valid source for reuse. This tightens the race to only
    +    happen when we need to close an open pack's file descriptor (c.f., the
    +    caller of `packfile.c::get_max_fd_limit()`) _and_ that pack was deleted,
    +    in which case we'll complain that a pack could not be accessed and
    +    die().
     
    -    The pack bitmap code does this, too, since prior to bab919bd44
    -    (pack-bitmap: check pack validity when opening bitmap, 2015-03-26) it
    +    The pack bitmap code does this, too, since prior to dc1daacdcc
    +    (pack-bitmap: check pack validity when opening bitmap, 2021-07-23) it
         was vulnerable to the same race.
     
         The MIDX bitmap code does not do this, and is vulnerable to the same
    -    race. Apply the same treatment as bab919bd44 to the routine responsible
    -    for opening multi-pack bitmaps to close this race.
    +    race. Apply the same treatment as dc1daacdcc to the routine responsible
    +    for opening the multi-pack bitmap's preferred pack to close this race.
     
    -    Similar to bab919bd44, we could technically just add this check in
    -    reuse_partial_packfile_from_bitmap(), since it's technically possible to
    -    use a MIDX .bitmap without needing to open any of its packs. But it's
    -    simpler to do the check as early as possible, covering all direct uses
    -    of the preferred pack. Note that doing this check early requires us to
    -    call prepare_midx_pack() early, too, so move the relevant part of that
    -    loop from load_reverse_index() into open_midx_bitmap_1().
    +    This patch handles the "preferred" pack (c.f., the section
    +    "multi-pack-index reverse indexes" in
    +    Documentation/technical/pack-format.txt) specially, since pack-objects
    +    depends on reusing exact chunks of that pack verbatim in
    +    reuse_partial_packfile_from_bitmap(). So if that pack cannot be loaded,
    +    the utility of a bitmap is significantly diminished.
    +
    +    Similar to dc1daacdcc, we could technically just add this check in
    +    reuse_partial_packfile_from_bitmap(), since it's possible to use a MIDX
    +    .bitmap without needing to open any of its packs. But it's simpler to do
    +    the check as early as possible, covering all direct uses of the
    +    preferred pack. Note that doing this check early requires us to call
    +    prepare_midx_pack() early, too, so move the relevant part of that loop
    +    from load_reverse_index() into open_midx_bitmap_1().
    +
    +    Subsequent patches handle the non-preferred packs in a slightly
    +    different fashion.
     
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
     +
     +	preferred = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];
     +	if (!is_pack_valid(preferred)) {
    -+		close(fd);
     +		warning(_("preferred pack (%s) is invalid"),
     +			preferred->pack_name);
     +		goto cleanup;
2:  9adf6e1341 < -:  ---------- builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects
-:  ---------- > 2:  2719d33f32 builtin/pack-objects.c: avoid redundant NULL check
-:  ---------- > 3:  cdc3265ec2 builtin/pack-objects.c: ensure included `--stdin-packs` exist
-:  ---------- > 4:  3fc3a95517 builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects
-- 
2.36.1.94.gb0d54bedca

  parent reply	other threads:[~2022-05-24 18:54 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-13 16:23 [PATCH 0/2] " Taylor Blau
2022-05-13 16:23 ` [PATCH 1/2] pack-bitmap: check preferred pack validity when opening MIDX bitmap Taylor Blau
2022-05-13 18:19   ` Junio C Hamano
2022-05-13 19:55     ` Taylor Blau
2022-05-13 16:23 ` [PATCH 2/2] builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects Taylor Blau
2022-05-13 23:06   ` Jonathan Tan
2022-05-14 13:17     ` Taylor Blau
2022-05-16  6:07       ` Jonathan Tan
2022-05-14 13:34     ` Taylor Blau
2022-05-16  6:11       ` Jonathan Tan
2022-05-24 18:54 ` Taylor Blau [this message]
2022-05-24 18:54   ` [PATCH v2 1/4] pack-bitmap.c: check preferred pack validity when opening MIDX bitmap Taylor Blau
2022-05-24 19:36     ` Ævar Arnfjörð Bjarmason
2022-05-24 21:38       ` Taylor Blau
2022-05-24 21:51         ` Ævar Arnfjörð Bjarmason
2022-05-24 18:54   ` [PATCH v2 2/4] builtin/pack-objects.c: avoid redundant NULL check Taylor Blau
2022-05-24 21:44     ` Junio C Hamano
2022-05-25  0:11       ` Taylor Blau
2022-05-24 18:54   ` [PATCH v2 3/4] builtin/pack-objects.c: ensure included `--stdin-packs` exist Taylor Blau
2022-05-24 19:46     ` Ævar Arnfjörð Bjarmason
2022-05-24 21:33       ` Taylor Blau
2022-05-24 21:49         ` Ævar Arnfjörð Bjarmason
2022-05-24 22:03     ` Junio C Hamano
2022-05-25  0:14       ` Taylor Blau
2022-05-26 19:21     ` Victoria Dye
2022-05-26 20:05       ` Taylor Blau
2022-05-24 18:54   ` [PATCH v2 4/4] builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects Taylor Blau
2022-05-24 21:38   ` [PATCH v2 0/4] pack-objects: fix a pair of MIDX bitmap-related races Junio C Hamano
2022-05-25  0:16     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1653418457.git.me@ttaylorr.com \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=vdye@github.com \
    --subject='Re: [PATCH v2 0/4] pack-objects: fix a pair of MIDX bitmap-related races' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).