From: Derrick Stolee <derrickstolee@github.com>
Date: Thu, 13 Apr 2023 13:49:34 GMT
Subject: Re: [PATCH v3 01/10] midx: fix segfault with no packs and invalid preferred pack
Message-ID: <1d2e1796-c305-0876-533c-2fe0fd39d722@github.com>
In-Reply-To: <dd8145beade440e5444130d3a3189b2c5d15a911.1681384405.git.ps@pks.im>

On 4/13/2023 7:16 AM, Patrick Steinhardt wrote:

> Fix this bug by resetting the preferred packfile index to `-1` before
> searching for the preferred pack. This fixes the segfault as we already
> check for whether the index is `> - 1`. If it is not, we simply don't
> pick a preferred packfile at all.

>  	if (preferred_pack_name) {
> -		int found = 0;
> +		ctx.preferred_pack_idx = -1;
> +

This patch looks good, but I did need to think about it for a bit,
so here are my thoughts (feel free to ignore):

I briefly considered recommending that we set this as the default in
an initializer macro, something like

  #define WRITE_MIDX_CONTEXT_INIT { .preferred_pack_idx = -1 }

but this struct is internal to the file and only constructed once
(at the start of write_midx_internal).

Further, outside the context of this patch we have this code:

	} else if (ctx.nr &&
		   (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) {
		struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;
		ctx.preferred_pack_idx = 0;

I don't see a way that ctx.preferred_pack_idx can be anything but zero
here, but it's also not going to give a segfault because of the 'ctx.nr'
condition.

Thanks,
-Stolee