| Message ID | 76585e5b1367a3adf18d761b2af9356ee64b46fd.1585962672.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge: learn --autostash | expand |
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(). > 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. > 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()))
Junio C Hamano <gitster@pobox.com> writes: >> 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. Ah, I was reading the state of applying the series thru to the end. At this point, there is no reset there, so you'd need to call reset to ensure that the next read_oneliner() on &buf will not append, but restart from the beginning. >> if (read_oneliner(&buf, rebase_path_squash_onto(), 0)) { >> - if (get_oid_hex(buf.buf, &opts->squash_onto) < 0) >> - return error(_("unusable squash-onto"));
Hi Junio, On Sun, Apr 05, 2020 at 02:33:08PM -0700, Junio C Hamano wrote: > 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. There is also a leak in the case where we don't take the early return since, before this patch, the read_oneliner() has no strbuf_release() following it which means buf gets leaked. > But that leak has nothing to do with the release called before > read_oneliner(). What I meant to say in my paragraph is essentially, "strbuf_release() is placed too early, there is a read_oneliner() after that nullifies the effects of strbuf_release()". I can rewrite this to be more clear. Thanks, Denton
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); 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; } if (!file_exists(git_path_opts_file()))
In read_populate_opts(), we call read_oneliner() _after_ calling strbuf_release(). This means that `buf` is leaked at the end of the function. 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(-)