Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 15, 2022, 19:54 UTC
Message-ID: <Y3Puhtj8Q8kf2wt0@nand.local>
In-Reply-To: <221115.86r0y4j8tr.gmgdl@evledraar.gmail.com>

On Tue, Nov 15, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> > I am disappointed in myself for finding this only after I pushed out a
> > hotfix to 'next' and rebuild the downstream branches.
> >
> > This should be a minimal fix on top of Ævar's patch to get 'next'
> > building again.
>
> I'm also disappointed in myself, sorry. I *did* test it locally with
> valgrind, but obviously fat-fingered it somehow and tested the wrong
> version. Sorry!

It's OK. Let's not beat ourselves up too much, and instead focusing on making sure the quality for the next pushout is higher (which is more on me than it is on you).

Show 16 quoted lines

> >  builtin/gc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index d87cf84041..38882a1e35 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
> >  	int found = 0;
> >  	struct string_list_item *item;
> >  	const struct string_list *list;
> > +	struct config_set cs = { { 0 } };
>
> Just "{ 0 }" here instead? I see it may have been copied from some older
> pre-image though, and they'll do the same in either case, so it's not
> important...

Copying from other zero-initializations of `struct config_set`:

    $ git grep -oh 'struct config_set.*= {.*' | sort | uniq -c
          3 struct config_set cs = { { 0 } };

Thanks, Taylor