Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack

From: Jeff King <peff@peff.net>
Date: Jun 19, 2024, 13:22 UTC
Message-ID: <20240619132208.GA895873@coredump.intra.peff.net>
In-Reply-To: <20240619130256.GA228005@coredump.intra.peff.net>

On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:

> I think it's a bug that fetch.unpackLimit causes index-pack to create a
> lockfile at all, but there's a more direct way to trigger the issue,
> which I've used in the patch below. I'll follow up with more details in
> a reply to the patch.

Your original test used transfer.unpackLimit. By itself that should just cause us to avoid calling unpack-objects, keeping the pack we got, but _not_ creating a .keep file. Likewise, if you pass one "-k" to fetch-pack, it should just keep the pack but without a .keep file (that's what the double "-k -k" does).

But both of these do trigger a .keep file, which seems wrong to me. The caller has no idea that the .keep file was created, so it won't clean it up, and the pack will be in limbo forever.

I tried bisecting and came up with fa74052922 (Always obtain fetch-pack arguments from struct fetch_pack_args, 2007-09-19). Given the length of time it's been this way, that makes me a little afraid to touch it. ;) But I think in practice it is not really triggered because of what I wrote earlier:

> Nobody noticed the bug for so long because the transport code used by
> "git fetch" always passes in a pack_lockfiles pointer, and remote-curl
> (the main user of the fetch-pack plumbing command) always passes
> --lock-pack.

That is, we're always asking for a lock-file anyway.

But it could affect external users of the fetch-pack plumbing. I.e., the very command that produced the segfault for you is probably leaving an unexpected .keep file in place.

Show 10 quoted lines

> So it's possible to ask index-pack to create the lock-file (using "-k
> -k") but not ask to record it (by avoiding "--lock-pack"). This worked
> fine until 5476e1efde (fetch-pack: print and use dangling .gitmodules,
> 2021-02-22), but now it causes a segfault.
> 
> Before that commit, if pack_lockfiles was NULL, we wouldn't bother
> reading the output from index-pack at all. But since that commit,
> index-pack may produce extra output if we asked it to fsck. So even if
> nobody cares about the lockfile path, we still need to read it to skip
> to the output we do care about.

There's another interesting fallout from 5476e1efde that I noticed here. Before that commit, if you did not pass --lock-pack to fetch-pack, then we would never bother reading stdout from index-pack, and it would go to the caller's stdout! So doing:

  git fetch-pack -k -k repo HEAD

would produce:

  keep	3282886e55735beb9a08b048394284b03bef8488

or similar on stdout. Which makes me wonder if some callers depend on that. Or if it is simply a bug, since it would be intermingled with fetch-pack's actual output. We still produce that output today, but if you use fetch.fsckObjects, then we eat it while looking for the other fsck-related output.

-Peff