Re: [PATCH 19/22] userdiff: fix leaking memory for configured diff drivers

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Wed, Aug 07, 2024 at 07:25:51PM +1000, James Liu wrote:
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]