Re: [PATCH v2 1/8] midx: fix segfault with no packs and invalid preferred pack

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 13, 2023, 09:28 UTC
Message-ID: <ZDfLMm-AoXkt-4zj@ncase>
In-Reply-To: <ZDbw6v8r89zciE3q@nand.local>

On Wed, Apr 12, 2023 at 01:56:58PM -0400, Taylor Blau wrote:

Show 26 quoted lines

> On Wed, Apr 12, 2023 at 12:22:31PM +0200, Patrick Steinhardt wrote:
> > Fix this bug by exiting early in case we have determined that the MIDX
> > wouldn't have any packfiles to index. While the request itself does not
> > make much sense anyway, it is still preferable to exit gracefully than
> > to abort.
> 
> Interesting. This reminded me quite a bit of eb57277ba3 (midx: prevent
> writing a .bitmap without any objects, 2022-02-09) which tackled a
> similar problem of trying to write a MIDX bitmap without any objects.
> 
> We may want to consider moving that conditional further up, since this
> makes the conditional added in eb57277ba3 dead code AFAICT. Here's a
> patch on top of this one that I think would do the trick.
> 
> It has the added benefit of sticking a:
> 
>     warning: unknown preferred pack: 'does-not-exist'
> 
> in the output before dying, which might be nice (though I doubt anybody
> will ever see it ;-)). The main difference is that we unset the bitmap
> related bits from `flags`, which avoids us trying to compute a preferred
> pack in the first place.
> 
> For it to work, though, we need to make sure that ctx.preferred_pack_idx
> is set to -1, and not zero-initialized, since we'll segfault otherwise
> when trying to read into an empty array.

Indeed, that is a good point. I think we can simplify your patch even further in that case:

diff --git a/midx.c b/midx.c
index 47989f7ea7..67eb617591 100644
--- a/midx.c
+++ b/midx.c
@@ -1328,17 +1328,17 @@ static int write_midx_internal(const char *object_dir,
        }

        if (preferred_pack_name) {
-               int found = 0;
+               ctx.preferred_pack_idx = -1;
+
                for (i = 0; i < ctx.nr; i++) {
                        if (!cmp_idx_or_pack_name(preferred_pack_name,
                                                  ctx.info[i].pack_name)) {
                                ctx.preferred_pack_idx = i;
-                               found = 1;
                                break;
                        }
                }

-               if (!found)
+               if (ctx.preferred_pack_idx == -1)
                        warning(_("unknown preferred pack: '%s'"),
                                preferred_pack_name);
        } else if (ctx.nr &&

The other cases already set `preferred_pack_idx = -1`, so this is really
all we need to do to fix the segfault.

Patrick