Re: [PATCH 21/21] list-objects-filter-options: work around reported leak on error
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 21, 2024, 08:45 UTC
- Message-ID
- <ZxYUkKAm0iBT8f5L@pks.im>
- In-Reply-To
- <875xppd3ct.fsf@iotcl.com>
On Fri, Oct 18, 2024 at 02:04:18PM +0200, Toon Claes wrote:
Show 44 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> > index 00611107d20..fa72e81e4ad 100644
> > --- a/list-objects-filter-options.c
> > +++ b/list-objects-filter-options.c
> > @@ -252,16 +252,14 @@ void parse_list_objects_filter(
> > const char *arg)
> > {
> > struct strbuf errbuf = STRBUF_INIT;
> > - int parse_error;
> >
> > if (!filter_options->filter_spec.buf)
> > BUG("filter_options not properly initialized");
> >
> > if (!filter_options->choice) {
> > + if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
> > + die("%s", errbuf.buf);
> > strbuf_addstr(&filter_options->filter_spec, arg);
> > -
> > - parse_error = gently_parse_list_objects_filter(
> > - filter_options, arg, &errbuf);
> > } else {
> > struct list_objects_filter_options *sub;
> >
> > @@ -271,18 +269,17 @@ void parse_list_objects_filter(
> > */
> > transform_to_combine_type(filter_options);
> >
> > - strbuf_addch(&filter_options->filter_spec, '+');
> > - filter_spec_append_urlencode(filter_options, arg);
> > ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
> > filter_options->sub_alloc);
> > sub = &filter_options->sub[filter_options->sub_nr - 1];
> >
> > list_objects_filter_init(sub);
> > - parse_error = gently_parse_list_objects_filter(sub, arg,
> > - &errbuf);
> > + if (gently_parse_list_objects_filter(sub, arg, &errbuf))
> > + die("%s", errbuf.buf);
>
> Do we actually have a test hitting this code path? I wanted to figure
> out why `filter_options->sub` wasn't leaky (I assume that's what you're
> talking about in your commit message), but I wasn't able to reproduce a
> scenario where we should die here.You only refer to the second hunk, right? I couldn't find any code paths hitting this, so this is more of a "Let's massage this code in the same way" change. I don't want the next person to go through the same rabbit hole as I had to go through :)
Patrick