Re: [PATCH 09/22] builtin/rebase: fix leaking `commit.gpgsign` value

[Patrick Steinhardt <ps@pks.im>]

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

On Wed, Aug 07, 2024 at 05:32:25PM +1000, James Liu wrote:
> On Tue Aug 6, 2024 at 7:00 PM AEST, Patrick Steinhardt wrote:
> > In `get_replay_opts()`, we unconditionally override the `gpg_sign` field
> > that already got populated by `sequencer_init_config()` in case the user
> > has "commit.gpgsign" set in their config. It is kind of dubious whether
> > this is the correct thing to do or a bug. What is clear though is that
> > this creates a memory leak.
> >
> > Let's mark this assignment with a TODO comment to figure out whether
> > this needs to be fixed or not. Meanwhile though, let's plug the memory
> > leak.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  builtin/rebase.c              | 8 ++++++++
> >  sequencer.c                   | 1 +
> >  t/t3404-rebase-interactive.sh | 1 +
> >  t/t3435-rebase-gpg-sign.sh    | 1 +
> >  t/t7030-verify-tag.sh         | 1 +
> >  5 files changed, 12 insertions(+)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index e3a8e74cfc..f65316a023 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -186,7 +186,15 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
> >  	replay.committer_date_is_author_date =
> >  					opts->committer_date_is_author_date;
> >  	replay.ignore_date = opts->ignore_date;
> > +
> > +	/*
> > +	 * TODO: Is it really intentional that we unconditionally override
> > +	 * `replay.gpg_sign` even if it has already been initialized via the
> > +	 * configuration?
> > +	 */
> > +	free(replay.gpg_sign);
> >  	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
> > +
> >  	replay.reflog_action = xstrdup(opts->reflog_action);
> >  	if (opts->strategy)
> >  		replay.strategy = xstrdup_or_null(opts->strategy);
> > diff --git a/sequencer.c b/sequencer.c
> > index 0291920f0b..cade9b0ca8 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -303,6 +303,7 @@ static int git_sequencer_config(const char *k, const char *v,
> >  	}
> >  
> >  	if (!strcmp(k, "commit.gpgsign")) {
> > +		free(opts->gpg_sign);
> >  		opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
> >  		return 0;
> >  	}
> 
> It looks like this free'ing would be managed by the caller by invoking
> `replay_opts_release()`, but it's not being done consistently.
> 
> For example, `do_interactive_rebase()` invokes `replay_opts_release()`,
> but `run_sequencer_rebase()` does not. Would it be better to address the
> leak here?

The problem here isn't that `replay_opts_release()` doesn't free the
values, or that the function isn't called consistently. The problem
rather is that we assign `opts->gpg_sign` even though it may already be
assigned an allocated string. Consequently, `replay_opts_release()`
doesn't even have a chance to free the old value because it cannot see
it anymore.

I'll massage the commit message a bit to clarify this.

Patrick

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