From: Phillip Wood Date: Tue, 22 Jun 2021 09:02:53 GMT Subject: Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle Message-ID: In-Reply-To: Hi Elijah On 21/06/2021 22:39, Elijah Newren wrote: > On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood wrote: >> >> Hi Andrzej >> >> Thanks for working on removing memory leaks from git. >> >> On 20/06/2021 16:12, andrzej@ahunt.org wrote: >>> From: Andrzej Hunt >>> >>> 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