Message ID | pull.1577.git.1692389061490.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | This fixes a minor memory leak (detected by LeakSanitizer) in git merge. | expand |
"Kevin Backhouse via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Kevin Backhouse <kevinbackhouse@github.com> > > To reproduce (with an ASAN build): > > ``` > mkdir test > cd test > git init > echo x > x.txt > git add . > git commit -m "WIP" > git checkout -b dev > echo y > x.txt > git add . > git commit -m "WIP" > git checkout main > echo z > x.txt > git add . > git commit -m "WIP" > echo a > x.txt > git add . > git merge dev > ``` We'd rather not to see the above in the proposed log message; can't we add (a variation of) it to our test suite? > The fix is to call free_commit_list(merge_bases) when an error occurs. We usually have the description of what the problem is and give an analysis on why/how it happens, before presenting a solution. Write it more like: The caller of merge_ort_recursive() expects the commit list passed in as the merge_bases parameter to be fully consumed by the function and does not free it when the function returns. In normal cases, the commit list does get consumed, but when the function returns early upon encountering an error, it forgets to clean it up. Fix this by freeing the list in the code paths for error returns. > merge-ort-wrappers.c | 4 +++- > merge-ort.c | 4 +++- These two places and their fixes seem OK, but I have to wonder if these are complete fixes. > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c > index 4acedf3c338..aeb56c9970c 100644 > --- a/merge-ort-wrappers.c > +++ b/merge-ort-wrappers.c > @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt, > struct tree *head = repo_get_commit_tree(opt->repo, side1); > struct merge_result tmp; > > - if (unclean(opt, head)) > + if (unclean(opt, head)) { > + free_commit_list(merge_bases); > return -1; > + } > > memset(&tmp, 0, sizeof(tmp)); > merge_incore_recursive(opt, merge_bases, side1, side2, &tmp); The function before this hunk appears to have very similar code structure. Does it need the same fix, or if not why not? > diff --git a/merge-ort.c b/merge-ort.c > index 8631c997002..a0eb91fb011 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt, > opt->branch1 = "Temporary merge branch 1"; > opt->branch2 = "Temporary merge branch 2"; > merge_ort_internal(opt, NULL, prev, next, result); > - if (result->clean < 0) > + if (result->clean < 0) { > + free_commit_list(merge_bases); > return; > + } Before this function, there is a comment that this came from another function and it seems to still have a very similar code structure. Does the other function need the same fix, or if not why not? Thanks.
On Fri, Aug 18, 2023 at 2:41 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Kevin Backhouse via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Kevin Backhouse <kevinbackhouse@github.com> > > > > To reproduce (with an ASAN build): > > > > ``` > > mkdir test > > cd test > > git init > > echo x > x.txt > > git add . > > git commit -m "WIP" > > git checkout -b dev > > echo y > x.txt > > git add . > > git commit -m "WIP" > > git checkout main > > echo z > x.txt > > git add . > > git commit -m "WIP" > > echo a > x.txt > > git add . > > git merge dev > > ``` > > We'd rather not to see the above in the proposed log message; can't > we add (a variation of) it to our test suite? > > > The fix is to call free_commit_list(merge_bases) when an error occurs. > > We usually have the description of what the problem is and give an > analysis on why/how it happens, before presenting a solution. Write > it more like: > > The caller of merge_ort_recursive() expects the commit list > passed in as the merge_bases parameter to be fully consumed by > the function and does not free it when the function returns. In > normal cases, the commit list does get consumed, but when the > function returns early upon encountering an error, it forgets to > clean it up. > > Fix this by freeing the list in the code paths for error returns. > > > merge-ort-wrappers.c | 4 +++- > > merge-ort.c | 4 +++- > > These two places and their fixes seem OK, but I have to wonder if > these are complete fixes. > > > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c > > index 4acedf3c338..aeb56c9970c 100644 > > --- a/merge-ort-wrappers.c > > +++ b/merge-ort-wrappers.c > > @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt, > > struct tree *head = repo_get_commit_tree(opt->repo, side1); > > struct merge_result tmp; > > > > - if (unclean(opt, head)) > > + if (unclean(opt, head)) { > > + free_commit_list(merge_bases); > > return -1; > > + } > > > > memset(&tmp, 0, sizeof(tmp)); > > merge_incore_recursive(opt, merge_bases, side1, side2, &tmp); > > The function before this hunk appears to have very similar code > structure. Does it need the same fix, or if not why not? > > > diff --git a/merge-ort.c b/merge-ort.c > > index 8631c997002..a0eb91fb011 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt, > > opt->branch1 = "Temporary merge branch 1"; > > opt->branch2 = "Temporary merge branch 2"; > > merge_ort_internal(opt, NULL, prev, next, result); > > - if (result->clean < 0) > > + if (result->clean < 0) { > > + free_commit_list(merge_bases); > > return; > > + } > > Before this function, there is a comment that this came from another > function and it seems to still have a very similar code structure. > Does the other function need the same fix, or if not why not? The other function would need a more involved fix, which would basically involve porting a59b8dd94f (merge-ort: fix memory leak in merge_ort_internal(), 2022-01-20) to merge-recursive as a preparatory step. This particular cleanup cannot be ported in its current form to merge-recursive.c until then.
diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c index 4acedf3c338..aeb56c9970c 100644 --- a/merge-ort-wrappers.c +++ b/merge-ort-wrappers.c @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt, struct tree *head = repo_get_commit_tree(opt->repo, side1); struct merge_result tmp; - if (unclean(opt, head)) + if (unclean(opt, head)) { + free_commit_list(merge_bases); return -1; + } memset(&tmp, 0, sizeof(tmp)); merge_incore_recursive(opt, merge_bases, side1, side2, &tmp); diff --git a/merge-ort.c b/merge-ort.c index 8631c997002..a0eb91fb011 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt, opt->branch1 = "Temporary merge branch 1"; opt->branch2 = "Temporary merge branch 2"; merge_ort_internal(opt, NULL, prev, next, result); - if (result->clean < 0) + if (result->clean < 0) { + free_commit_list(merge_bases); return; + } opt->branch1 = saved_b1; opt->branch2 = saved_b2; opt->priv->call_depth--;