Thanks to Junio for his comments on V1. Changes since V1: * Patch 1 is new and addresses a memory leak noticed by Junio * Patch 2 is unchanged * Patch 3 now avoids a double free of xe1 on error * Patch 4 reworked handling of the return value V1 Cover Letter: Other users of xdiff such as libgit2 need to be able to handle allocation failures. This series fixes the few cases where we are not doing this already. Edward's patch[1] reminded me that I had these waiting to be submitted. [1] https://lore.kernel.org/git/20220209013354.GB7@abe733c6e288/ Phillip Wood (4): xdiff: fix a memory leak xdiff: handle allocation failure in patience diff xdiff: refactor a function xdiff: handle allocation failure when merging xdiff/xdiffi.c | 33 +++++++++++++++++---------------- xdiff/xhistogram.c | 3 --- xdiff/xmerge.c | 42 ++++++++++++++++++++++-------------------- xdiff/xpatience.c | 21 ++++++++++++--------- 4 files changed, 51 insertions(+), 48 deletions(-) base-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1140%2Fphillipwood%2Fwip%2Fxdiff-handle-allocation-failures-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1140/phillipwood/wip/xdiff-handle-allocation-failures-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1140 Range-diff vs v1: -: ----------- > 1: 3fcb6c80367 xdiff: fix a memory leak 1: b8f88f1b9f8 = 2: fec1f4a4152 xdiff: handle allocation failure in patience diff 2: 8655bb0348d ! 3: 073a45e9e85 xdiff: refactor a function @@ Metadata ## Commit message ## xdiff: refactor a function - 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. + Use the standard "goto out" pattern rather than repeating very similar + code after checking for each error. This will simplify the next commit + that starts handling allocation failures that are currently ignored. + On error xdl_do_diff() frees the environment so we need to take care + to avoid a double free in that case. xdl_build_script() does not + assign a result unless it is successful so there is no possibility of + a double free if it fails. Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> @@ xdiff/xmerge.c: static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, 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 }; + xdfenv_t xe1, xe2; +- int status; + int status = -1; xpparam_t const *xpp = &xmp->xpp; @@ xdiff/xmerge.c: static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, result->size = 0; - if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) { -- return -1; ++ 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; ++ goto free_xe1; /* avoid double free of xe2 */ + if (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 || xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 || @@ xdiff/xmerge.c: int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, xdl_free_script(xscr1); xdl_free_script(xscr2); +- xdl_free_env(&xe1); + xdl_free_env(&xe2); ++ free_xe1: ++ xdl_free_env(&xe1); + + return status; + } 3: 3e935abba1d ! 4: 7e705d771b0 xdiff: handle allocation failure when merging @@ Commit message ## xdiff/xmerge.c ## @@ xdiff/xmerge.c: int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, - status = 0; + xdl_build_script(&xe2, &xscr2) < 0) + goto out; + +- status = 0; if (!xscr1) { result->ptr = xdl_malloc(mf2->size); -+ if (!result->ptr) { -+ status = -1; ++ if (!result->ptr) + goto out; -+ } ++ status = 0; memcpy(result->ptr, mf2->ptr, mf2->size); result->size = mf2->size; } else if (!xscr2) { result->ptr = xdl_malloc(mf1->size); -+ if (!result->ptr) { -+ status = -1; ++ if (!result->ptr) + goto out; -+ } ++ status = 0; memcpy(result->ptr, mf1->ptr, mf1->size); result->size = mf1->size; } else { -- gitgitgadget
