| Message ID | a6fec1ab-4006-2896-a2d9-dc0d8fa1c132@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1533bda7700d6aa4374509f9611ae52a4eb2bcda |
| Headers | show |
| Series | [01/11] rev-parse: fix a leak with --abbrev-ref | expand |
On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote: > In e89f151db1 (branch: move --set-upstream-to behavior to > dwim_and_setup_tracking(), 2022-01-28) the string returned by > dwim_branch_start() was not freed, resulting in a memory leak. Yep, looks good. One small comment: > diff --git a/branch.c b/branch.c > index ba3914adf5..a7333a4c32 100644 > --- a/branch.c > +++ b/branch.c > @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref, > const char *orig_ref, enum branch_track track, > int quiet) > { > - char *real_orig_ref; > + char *real_orig_ref = NULL; > dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL); > setup_tracking(new_ref, real_orig_ref, track, quiet); > + free(real_orig_ref); > } Adding the NULL-initialization signals that we expect that real_orig_ref might sometimes _not_ be filled in by dwim_branch_start(). But I think it always will be; and if it weren't, that would make passing it to setup_tracking() rather suspicious. So I don't think the NULL initialization is wrong, and certainly it is more defensive. But I find it a little misleading. -Peff
On 11-jun-2023 23:21:06, Jeff King wrote: > On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote: > > > In e89f151db1 (branch: move --set-upstream-to behavior to > > dwim_and_setup_tracking(), 2022-01-28) the string returned by > > dwim_branch_start() was not freed, resulting in a memory leak. > > Yep, looks good. One small comment: > > > diff --git a/branch.c b/branch.c > > index ba3914adf5..a7333a4c32 100644 > > --- a/branch.c > > +++ b/branch.c > > @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref, > > const char *orig_ref, enum branch_track track, > > int quiet) > > { > > - char *real_orig_ref; > > + char *real_orig_ref = NULL; > > dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL); > > setup_tracking(new_ref, real_orig_ref, track, quiet); > > + free(real_orig_ref); > > } > > Adding the NULL-initialization signals that we expect that real_orig_ref > might sometimes _not_ be filled in by dwim_branch_start(). But I think > it always will be; and if it weren't, that would make passing it to > setup_tracking() rather suspicious. > > So I don't think the NULL initialization is wrong, and certainly it is > more defensive. But I find it a little misleading. I don't have any objection to this. But I've seen Junio has already merged this to 'next'. I'm not going to re-roll this commit. Sorry. Thank you for your review.
diff --git a/branch.c b/branch.c index ba3914adf5..a7333a4c32 100644 --- a/branch.c +++ b/branch.c @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref, const char *orig_ref, enum branch_track track, int quiet) { - char *real_orig_ref; + char *real_orig_ref = NULL; dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL); setup_tracking(new_ref, real_orig_ref, track, quiet); + free(real_orig_ref); } /**
In e89f151db1 (branch: move --set-upstream-to behavior to dwim_and_setup_tracking(), 2022-01-28) the string returned by dwim_branch_start() was not freed, resulting in a memory leak. It can be reviewed with: $ git remote add local . $ git update-ref refs/remotes/local/foo HEAD $ git branch --set-upstream-to local/foo foo Direct leak of 23 byte(s) in 1 object(s) allocated from: ... in xstrdup wrapper.c ... in expand_ref refs.c ... in repo_dwim_ref refs.c ... in dwim_branch_start branch.c ... in dwim_and_setup_tracking branch.c ... in cmd_branch builtin/branch.c ... in run_builtin git.c Let's free it now. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- branch.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)