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?
