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@xxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
