Hi Elijah
On 21/06/2021 22:39, Elijah Newren wrote:
On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
Hi Andrzej
Thanks for working on removing memory leaks from git.
On 20/06/2021 16:12, andrzej@xxxxxxxxx wrote:
From: Andrzej Hunt <ajrhunt@xxxxxxxxxx>
This change:
- xstrdup()'s all string being used for replace_opts.strategy, to
I think you mean replay_opts rather than replace_opts.
guarantee that replace_opts owns these strings. This is needed because
sequencer_remove_state() will free replace_opts.strategy, and it's
usually called as part of the usage of replace_opts.
- Removes xstrdup()'s being used to populate options.strategy in
cmd_rebase(), which avoids leaking options.strategy, even in the
case where strategy is never moved/copied into replace_opts.
These changes are needed because:
- We would always create a new string for options.strategy if we either
get a strategy via options (OPT_STRING(...strategy...), or via
GIT_TEST_MERGE_ALGORITHM.
- But only sometimes is this string copied into replace_opts - in which
case it did get free()'d in sequencer_remove_state().
- The rest of the time, the newly allocated string would remain unused,
causing a leak. But we can't just add a free because that can result
in a double-free in those cases where replace_opts was populated.
An alternative approach would be to set options.strategy to NULL when
moving the pointer to replace_opts.strategy, combined with always
free()'ing options.strategy, but that seems like a more
complicated and wasteful approach.
read_basic_state() contains
if (file_exists(state_dir_path("strategy", opts))) {
strbuf_reset(&buf);
if (!read_oneliner(&buf, state_dir_path("strategy", opts),
READ_ONELINER_WARN_MISSING))
return -1;
free(opts->strategy);
opts->strategy = xstrdup(buf.buf);
}
So we do try to free opts->strategy when reading the state from disc and
we allocate a new string. I suspect that opts->strategy is actually NULL
in when this function is called but I haven't checked. Given that we are
allocating a copy above I think maybe your alternative approach of
always freeing opts->strategy would be better.
Good catches. sequencer_remove_state() in sequencer.c also has a
free(opts->strategy) call.
To make things even more muddy, we have code like
replay.strategy = replay.default_strategy;
or
opts->strategy = opts->default_strategy;
which both will probably work really poorly with the calls to
free(opts->default_strategy);
free(opts->strategy);
from sequencer_remove_state(). I suspect we've got a few bugs here...
It's not immediately obvious but I think those are actually safe.
opts->default_strategy is allocated by sequencer_init_config() so it is
correct to free it and when we assign it in rebase.c we do
else if (!replay.strategy && replay.default_strategy) {
replay.strategy = replay.default_strategy;
replay.default_strategy = NULL;
}
so there is no double free. There is similar code in builtin/revert.c
which I think is where your other example came from. I think there is a
leak in builtin/revert.c though
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
opts->default_strategy = NULL;
}
/* do some other stuff */
/* These option values will be free()d */
opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
opts->strategy = xstrdup_or_null(opts->strategy);
So we copy the default strategy, leaking the original copy from
sequencer_init_options() if --strategy isn't given on the command line.
I think it would be simple to fix this by making the copy earlier.
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
opts->default_strategy = NULL;
} else if (opts->strategy) {
/* This option will be free()d in sequencer_remove_state() */
opts->strategy = xstrdup(opts->strategy);
}
I'm going offline for a week or so in a couple of days but I'll have
look at making a proper patch when I get back.
Best Wishes
Phillip
