| Message ID | 8655bb0348d7344ae85c8d521fb1ec7a5f4188d2.1644404356.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xdiff: handle allocation failures | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Rather than having to remember exactly what to free after an > allocation failure use the standard "goto out" pattern. This will > simplify the next commit that starts handling currently unhandled > failures. It sound like this is meant to be a no-op clean-up; let's see. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > xdiff/xmerge.c | 36 ++++++++++++++++-------------------- > 1 file changed, 16 insertions(+), 20 deletions(-) > > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c > index fff0b594f9a..48c5e9e3f35 100644 > --- a/xdiff/xmerge.c > +++ b/xdiff/xmerge.c > @@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, > int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, > xmparam_t const *xmp, mmbuffer_t *result) > { > - xdchange_t *xscr1, *xscr2; > - xdfenv_t xe1, xe2; > - int status; > + xdchange_t *xscr1 = NULL, *xscr2 = NULL; > + xdfenv_t xe1 = { 0 }, xe2 = { 0 }; > + int status = -1; > xpparam_t const *xpp = &xmp->xpp; > > result->ptr = NULL; > result->size = 0; > > - if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) { > - return -1; > - } > - if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) { > - xdl_free_env(&xe1); > - return -1; > - } OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I didn't check if patience and histogram also do so correctly), so the original was not leaking xe1 or xe2. > + if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) > + goto out; > + > + if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) > + goto out; > + And this does not change that. > if (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 || > xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 || > - xdl_build_script(&xe1, &xscr1) < 0) { > - xdl_free_env(&xe1); > - return -1; > - } > + xdl_build_script(&xe1, &xscr1) < 0) > + goto out; > + Here, as xdl_build_script() does free the script it failed to build, but not the xe it was given, the original is correct to free xe1. We jump from here to leave the freeing of xe1 to the clean-up part. > if (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 || > xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 || > - xdl_build_script(&xe2, &xscr2) < 0) { > - xdl_free_script(xscr1); > - xdl_free_env(&xe1); > - xdl_free_env(&xe2); > - return -1; > - } > + xdl_build_script(&xe2, &xscr2) < 0) > + goto out; Ditto for xe2 here. > status = 0; > if (!xscr1) { > result->ptr = xdl_malloc(mf2->size); > @@ -727,6 +722,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, > &xe2, xscr2, > xmp, result); > } > + out: > xdl_free_script(xscr1); > xdl_free_script(xscr2); And after the post-context of this hunk, we do free_env() both xe1 and xe2, so we should be doing the same thing as before. Looking good.
Junio C Hamano <gitster@pobox.com> writes: >> int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, >> xmparam_t const *xmp, mmbuffer_t *result) >> { >> - xdchange_t *xscr1, *xscr2; >> - xdfenv_t xe1, xe2; >> - int status; >> + xdchange_t *xscr1 = NULL, *xscr2 = NULL; >> + xdfenv_t xe1 = { 0 }, xe2 = { 0 }; >> + int status = -1; >> xpparam_t const *xpp = &xmp->xpp; >> >> result->ptr = NULL; >> result->size = 0; >> >> - if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) { >> - return -1; >> - } >> - if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) { >> - xdl_free_env(&xe1); >> - return -1; >> - } > > OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I > didn't check if patience and histogram also do so correctly), so the > original was not leaking xe1 or xe2. After I wrote the above, I took a brief look at patience and histogram, and it does not seem to release resources held by "env" when it signals a failure by returning a negative value. So it seems that the original used with patience or histogram were leaking env when it failed, and this patch plugs that small leak. If that is indeed the case, please note it in the proposed log message. Thanks.
Hi Junio On 10/02/2022 06:28, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >>> int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, >>> xmparam_t const *xmp, mmbuffer_t *result) >>> { >>> - xdchange_t *xscr1, *xscr2; >>> - xdfenv_t xe1, xe2; >>> - int status; >>> + xdchange_t *xscr1 = NULL, *xscr2 = NULL; >>> + xdfenv_t xe1 = { 0 }, xe2 = { 0 }; >>> + int status = -1; >>> xpparam_t const *xpp = &xmp->xpp; >>> >>> result->ptr = NULL; >>> result->size = 0; >>> >>> - if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) { >>> - return -1; >>> - } >>> - if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) { >>> - xdl_free_env(&xe1); >>> - return -1; >>> - } >> >> OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I >> didn't check if patience and histogram also do so correctly), so the >> original was not leaking xe1 or xe2. > > After I wrote the above, I took a brief look at patience and histogram, > and it does not seem to release resources held by "env" when it signals > a failure by returning a negative value. So it seems that the original > used with patience or histogram were leaking env when it failed, and > this patch plugs that small leak. > > If that is indeed the case, please note it in the proposed log > message. Oh well spotted, xdl_do_diff() only frees "env" if the myers algorithm has an error, if the patience or histogram algorithms have an error then they do not free "env" and it is not freed by xdl_do_diff(). This patch inadvertently fixes that leak when merging but not when calling xdl_do_diff() to compact conflicts in zealous mode or when doing a plain diff. I think the simplest fix is to have xdl_do_diff() free "env" when there is an error what ever algorithm is used. I'll try to put a patch together to fix the other cases. If we fix this leak in xdl_do_diff() then maybe we should go back to returning -1 in the hunk above and explain in the log message why that is ok. Best Wishes Phillip > Thanks.
Phillip Wood <phillip.wood123@gmail.com> writes: > Oh well spotted, xdl_do_diff() only frees "env" if the myers algorithm > has an error, if the patience or histogram algorithms have an error > then they do not free "env" and it is not freed by xdl_do_diff(). This > patch inadvertently fixes that leak when merging but not when calling > xdl_do_diff() to compact conflicts in zealous mode or when doing a > plain diff. I think the simplest fix is to have xdl_do_diff() free > "env" when there is an error what ever algorithm is used. Heh, I didn't even look at the other uses of xdl_do_diff(); I am glad you did. > I'll try to put a patch together to fix the other cases. If we fix > this leak in xdl_do_diff() then maybe we should go back to returning > -1 in the hunk above and explain in the log message why that is ok. Thanks.
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index fff0b594f9a..48c5e9e3f35 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, xmparam_t const *xmp, mmbuffer_t *result) { - xdchange_t *xscr1, *xscr2; - xdfenv_t xe1, xe2; - int status; + xdchange_t *xscr1 = NULL, *xscr2 = NULL; + xdfenv_t xe1 = { 0 }, xe2 = { 0 }; + int status = -1; xpparam_t const *xpp = &xmp->xpp; result->ptr = NULL; result->size = 0; - if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) { - return -1; - } - if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) { - xdl_free_env(&xe1); - return -1; - } + if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) + goto out; + + if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) + goto out; + if (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 || xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 || - xdl_build_script(&xe1, &xscr1) < 0) { - xdl_free_env(&xe1); - return -1; - } + xdl_build_script(&xe1, &xscr1) < 0) + goto out; + if (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 || xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 || - xdl_build_script(&xe2, &xscr2) < 0) { - xdl_free_script(xscr1); - xdl_free_env(&xe1); - xdl_free_env(&xe2); - return -1; - } + xdl_build_script(&xe2, &xscr2) < 0) + goto out; + status = 0; if (!xscr1) { result->ptr = xdl_malloc(mf2->size); @@ -727,6 +722,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, &xe2, xscr2, xmp, result); } + out: xdl_free_script(xscr1); xdl_free_script(xscr2);