| Message ID | 05290fc1f14cae8229c42f2d0aafe6619c069e3a.1722933642.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
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?
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
Hi Patrick On 06/08/2024 10:00, Patrick Steinhardt wrote: > @@ -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); > + The code that handles "-S" could certainly be clearer. The value returned from the config is either "" or NULL, not a key name. In cmd_main() options.gpg_sign_opt is initialized by rebase_config(), we set gpg_sign to "" if options.gpg_sign_opt is non-NULL, free options.gpg_sign_opt and then copy gpg_sign back into options.gpg_sign_opt after parsing the command line so we're not losing anything by unconditionally copying it here. The code changes look good, though I'm not sure we need to add the blank lines. It's always nice to see more tests marked as leak-free especially a big file like t3404. Best Wishes Phillip
On Thu, Aug 08, 2024 at 11:07:53AM +0100, Phillip Wood wrote: > Hi Patrick > > On 06/08/2024 10:00, Patrick Steinhardt wrote: > > @@ -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); > > + > > The code that handles "-S" could certainly be clearer. The value returned > from the config is either "" or NULL, not a key name. In cmd_main() > options.gpg_sign_opt is initialized by rebase_config(), we set gpg_sign to > "" if options.gpg_sign_opt is non-NULL, free options.gpg_sign_opt and then > copy gpg_sign back into options.gpg_sign_opt after parsing the command line > so we're not losing anything by unconditionally copying it here. The code > changes look good, though I'm not sure we need to add the blank lines. It's > always nice to see more tests marked as leak-free especially a big file like > t3404. Okay. In that case I'll just drop the comment. Thanks! Patrick
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; } diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f92baad138..f171af3061 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -26,6 +26,7 @@ Initial setup: touch file "conflict". ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh index 6aa2aeb628..6e329fea7c 100755 --- a/t/t3435-rebase-gpg-sign.sh +++ b/t/t3435-rebase-gpg-sign.sh @@ -8,6 +8,7 @@ test_description='test rebase --[no-]gpg-sign' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-rebase.sh" . "$TEST_DIRECTORY/lib-gpg.sh" diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 6f526c37c2..effa826744 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -4,6 +4,7 @@ test_description='signed tag tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh"
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(+)