Denton Liu <liu.denton@xxxxxxxxx> writes:
> In read_populate_opts(), we call read_oneliner() _after_ calling
> strbuf_release(). This means that `buf` is leaked at the end of the
> function.
I do not think the above makes much sense. _release() will free the
piece of memory occupied by .buf and reinitializes the strbuf, and
in doing so there is no leak. read_oneliner() called after it
allocates and reads into there. Freeing the resource needs to be
done after the caller is done with what read_oneliner() has read.
There is a leak, because read_oneliner() gets called and before the
code has a chance to do strbuf_reease() there is an error return.
That does not have anything to do with the call to strbuf_release()
in the middle of the function.
But that leak has nothing to do with the release called before
read_oneliner().
> Always clean up the strbuf by going to `done_rebase_i` whether or not
> we return an error.
>
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> sequencer.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e528225e78..faab0b13e8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2485,6 +2485,7 @@ static int read_populate_opts(struct replay_opts *opts)
> {
> if (is_rebase_i(opts)) {
> struct strbuf buf = STRBUF_INIT;
> + int ret = 0;
>
> if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
> if (!starts_with(buf.buf, "-S"))
> @@ -2525,7 +2526,7 @@ static int read_populate_opts(struct replay_opts *opts)
> opts->keep_redundant_commits = 1;
>
> read_strategy_opts(opts, &buf);
> - strbuf_release(&buf);
> + strbuf_reset(&buf);
As read_oneliner() has a strbuf_reset() at the beginning *anyway*,
why not just get rid of the call to _release() here instead? After
all, there is no _release() or _reset() before the call to read_oneliner()
in the next hunk, or two calls to read_oneliner() inside the
read_strategy_opts() function called in the above.
> if (read_oneliner(&opts->current_fixups,
> rebase_path_current_fixups(), 1)) {
> @@ -2538,12 +2539,16 @@ static int read_populate_opts(struct replay_opts *opts)
> }
>
> if (read_oneliner(&buf, rebase_path_squash_onto(), 0)) {
> - if (get_oid_hex(buf.buf, &opts->squash_onto) < 0)
> - return error(_("unusable squash-onto"));
> + if (get_oid_hex(buf.buf, &opts->squash_onto) < 0) {
> + ret = error(_("unusable squash-onto"));
> + goto done_rebase_i;
> + }
> opts->have_squash_onto = 1;
> }
>
> - return 0;
> +done_rebase_i:
> + strbuf_release(&buf);
> + return ret;
This part indeed IS the right fix to the existing leak.
> }
>
> if (!file_exists(git_path_opts_file()))
