Re: [PATCH v4 03/23] sequencer: stop leaking buf
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 5, 2020, 21:33 UTC
- Message-ID
- <xmqqzhbpwrcb.fsf@gitster.c.googlers.com>
- In-Reply-To
- <76585e5b1367a3adf18d761b2af9356ee64b46fd.1585962672.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> 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().
Show 26 quoted lines
> 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@gmail.com>
> ---
> 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.
Show 19 quoted lines
> 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()))