Re: [PATCH 18/22] builtin/format-patch: fix various trivial memory leaks

[Patrick Steinhardt <ps@pks.im>]

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

On Wed, Aug 07, 2024 at 06:51:52PM +1000, James Liu wrote:
> > diff --git a/builtin/log.c b/builtin/log.c
> > index a73a767606..ff997a0d0e 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -2023,6 +2024,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  	const char *rfc = NULL;
> >  	int creation_factor = -1;
> >  	const char *signature = git_version_string;
> > +	char *signature_to_free = NULL;
> >  	char *signature_file_arg = NULL;
> >  	struct keep_callback_data keep_callback_data = {
> >  		.cfg = &cfg,
> > @@ -2443,7 +2445,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  
> >  		if (strbuf_read_file(&buf, signature_file, 128) < 0)
> >  			die_errno(_("unable to read signature file '%s'"), signature_file);
> > -		signature = strbuf_detach(&buf, NULL);
> > +		signature = signature_to_free = strbuf_detach(&buf, NULL);
> 
> Do I understand this correctly, that the multiple assignment here allows
> us to maintain a reference to the pointer returned by `strbuf_detach()`
> in `signature_to_free`, and we do this because `signature` can take on a
> different value below?

Not only below, but also its default value is `git_version_string`,
which is a string constant. So we use the multiple assignment such that
we can avoid freeing `signature`, which may contain string constants,
and unconditionally free `signature_to_free` because that variable
always holds an allocated string or a `NULL` pointer.

Patrick

> >  	} else if (cfg.signature) {
> >  		signature = cfg.signature;
> >  	}
> 
> 

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