Re: with reuse-delta patches, fetching with bitmaps segfaults due to possibly incomplete bitmap traverse

From: Siddharth Agarwal <sid0@fb.com>
Date: Mar 24, 2014, 00:01 UTC
Message-ID: <532F75BC.7080301@fb.com>
In-Reply-To: <20140322125626.GA22890@sigill.intra.peff.net>

On 03/22/2014 05:56 AM, Jeff King wrote:

Show 6 quoted lines

> On Fri, Mar 21, 2014 at 07:58:55PM -0700, Siddharth Agarwal wrote:
>
> Is it also reproducible just with the tip of "next"? Note that the
> patches in jk/bitmap-reuse-delta have not been widely deployed (in
> particular, we are not yet using them at GitHub, and we track segfaults
> on our servers closely and have not seen any related to this).

I cannot reproduce this with the tip of next (tested with 4443bfd). That's also -- unsurprisingly -- significantly slower in the compression phase and sends much more data (3x for the pair of repos in the OP) over the wire than a Git that doesn't use bitmaps.

> Those patches allocate extra "fake" entries in the entry->delta fields,
> which are not accounted for in to_pack.nr_objects. It's entirely
> possible that those entries are related to the bug you are seeing.

That sounds like it could be the problem, yes.

Show 12 quoted lines

> Hmm, yeah, that confirms my suspicion. In the earlier loops, we call
> add_to_write_order, which only adds the object in question, and can
> never exceed to_pack.nr_objects. In this final loop, we call
> add_family_to_write_order, which is going to add any deltas that were
> not already included.
>
> The patch below may fix your problem, but I have a feeling it is not the
> right thing to do. The point of 81cdec28 is to try to point to a delta
> entry as if it were a "preferred base" (i.e., something we know that the
> other side has already). We perhaps want to add these entries to the
> actual packing list, and skip them as we do with normal preferred_base
> objects.

The patch does stop Git from segfaulting. I know too little to judge its correctness, though.

Show 15 quoted lines

>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 9fc5321..ca1b0f7 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1437,6 +1437,7 @@ static void check_object(struct object_entry *entry)
>   			entry->delta = xcalloc(1, sizeof(*entry->delta));
>   			hashcpy(entry->delta->idx.sha1, base_ref);
>   			entry->delta->preferred_base = 1;
> +			entry->delta->filled = 1;
>   			unuse_pack(&w_curs);
>   			return;
>   		}
>
> -Peff