Re: [PATCH 19/22] userdiff: fix leaking memory for configured diff drivers
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 8, 2024, 05:05 UTC
- Message-ID
- <ZrRSDaCClfh9dMZK@tanuki>
- In-Reply-To
- <D39KL8N6S0HC.HFB9VRAVWYOX@jamesliu.io>
On Wed, Aug 07, 2024 at 07:25:51PM +1000, James Liu wrote:
Show 31 quoted lines
> > Refactor the code such that we have two pointers for each of these
> > strings: one that holds the value as accessed by other subsystems, and
> > one that points to the same string in case it has been allocated. Like
> > this, we can safely free the second pointer and thus plug those memory
> > leaks.
> >
> > diff --git a/userdiff.c b/userdiff.c
> > index c4ebb9ff73..989629149f 100644
> > --- a/userdiff.c
> > +++ b/userdiff.c
> > @@ -399,8 +399,11 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *name, size_t
> > static int parse_funcname(struct userdiff_funcname *f, const char *k,
> > const char *v, int cflags)
> > {
> > - if (git_config_string((char **) &f->pattern, k, v) < 0)
> > + f->pattern = NULL;
> > + FREE_AND_NULL(f->pattern_owned);
> > + if (git_config_string(&f->pattern_owned, k, v) < 0)
> > return -1;
> > + f->pattern = f->pattern_owned;
> > f->cflags = cflags;
> > return 0;
> > }
>
> I'm not sure if I understand this change completely. We don't seem to be
> using `pattern_owned` (and the other *_owned) fields differently from
> their regular counterparts.
>
> Is it because we can't do the following?
>
> FREE_AND_NULL((char **)f->pattern);Yup. We have a bunch of statically defined userdiff drivers, all of which use string constants as patterns. We thus cannot reliably free those and instead have to track the allocated strings in a separate variable.
Patrick