{"thread":{"id":"57391","subject":"[PATCH 0/3] xdiff: handle allocation failures","startedAt":"2022-02-09T11:58:21Z","lastAt":"2022-02-17T11:05:57Z","messageCount":18,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Phillip Wood","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"448037","messageId":"pull.1140.git.1644404356.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":null,"subject":"[PATCH 0/3] xdiff: handle allocation failures","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T10:59:12Z","receivedAt":"2022-02-09T11:58:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Other users of xdiff such as libgit2 need to be able to handle allocation\nfailures. This series fixes the few cases where we are not doing this\nalready.\n\nEdward's patch[1] reminded me that I had these waiting to be submitted.\n\n[1] https://lore.kernel.org/git/20220209013354.GB7@abe733c6e288/\n\nPhillip Wood (3):\n  xdiff: handle allocation failure in patience diff\n  xdiff: refactor a function\n  xdiff: handle allocation failure when merging\n\n xdiff/xmerge.c    | 44 ++++++++++++++++++++++++--------------------\n xdiff/xpatience.c | 17 ++++++++++++-----\n 2 files changed, 36 insertions(+), 25 deletions(-)\n\n\nbase-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1140%2Fphillipwood%2Fwip%2Fxdiff-handle-allocation-failures-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1140/phillipwood/wip/xdiff-handle-allocation-failures-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1140\n-- \ngitgitgadget\n"},{"id":"448038","messageId":"3e935abba1d1ea6cbf10883acf47edcb0162105c.1644404356.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.git.1644404356.gitgitgadget@gmail.com","subject":"[PATCH 3/3] xdiff: handle allocation failure when merging","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T10:59:15Z","receivedAt":"2022-02-09T11:58:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nOther users of xdiff such as libgit2 need to be able to handle\nallocation failures. These allocation failures were previously\nignored.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xmerge.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 48c5e9e3f35..b9d72f0e9f8 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -711,10 +711,18 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \tstatus = 0;\n \tif (!xscr1) {\n \t\tresult->ptr = xdl_malloc(mf2->size);\n+\t\tif (!result->ptr) {\n+\t\t\tstatus = -1;\n+\t\t\tgoto out;\n+\t\t}\n \t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n \t\tresult->size = mf2->size;\n \t} else if (!xscr2) {\n \t\tresult->ptr = xdl_malloc(mf1->size);\n+\t\tif (!result->ptr) {\n+\t\t\tstatus = -1;\n+\t\t\tgoto out;\n+\t\t}\n \t\tmemcpy(result->ptr, mf1->ptr, mf1->size);\n \t\tresult->size = mf1->size;\n \t} else {\n-- \ngitgitgadget\n"},{"id":"448039","messageId":"8655bb0348d7344ae85c8d521fb1ec7a5f4188d2.1644404356.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.git.1644404356.gitgitgadget@gmail.com","subject":"[PATCH 2/3] xdiff: refactor a function","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T10:59:14Z","receivedAt":"2022-02-09T11:58:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nRather than having to remember exactly what to free after an\nallocation failure use the standard \"goto out\" pattern. This will\nsimplify the next commit that starts handling currently unhandled\nfailures.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xmerge.c | 36 ++++++++++++++++--------------------\n 1 file changed, 16 insertions(+), 20 deletions(-)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex fff0b594f9a..48c5e9e3f35 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t\txmparam_t const *xmp, mmbuffer_t *result)\n {\n-\txdchange_t *xscr1, *xscr2;\n-\txdfenv_t xe1, xe2;\n-\tint status;\n+\txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n+\txdfenv_t xe1 = { 0 }, xe2 = { 0 };\n+\tint status = -1;\n \txpparam_t const *xpp = &xmp->xpp;\n \n \tresult->ptr = NULL;\n \tresult->size = 0;\n \n-\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n-\t\treturn -1;\n-\t}\n-\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n-\t\txdl_free_env(&xe1);\n-\t\treturn -1;\n-\t}\n+\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0)\n+\t\tgoto out;\n+\n+\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0)\n+\t\tgoto out;\n+\n \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n-\t    xdl_build_script(&xe1, &xscr1) < 0) {\n-\t\txdl_free_env(&xe1);\n-\t\treturn -1;\n-\t}\n+\t    xdl_build_script(&xe1, &xscr1) < 0)\n+\t\tgoto out;\n+\n \tif (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||\n-\t    xdl_build_script(&xe2, &xscr2) < 0) {\n-\t\txdl_free_script(xscr1);\n-\t\txdl_free_env(&xe1);\n-\t\txdl_free_env(&xe2);\n-\t\treturn -1;\n-\t}\n+\t    xdl_build_script(&xe2, &xscr2) < 0)\n+\t\tgoto out;\n+\n \tstatus = 0;\n \tif (!xscr1) {\n \t\tresult->ptr = xdl_malloc(mf2->size);\n@@ -727,6 +722,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t\t\t\t      &xe2, xscr2,\n \t\t\t\t      xmp, result);\n \t}\n+ out:\n \txdl_free_script(xscr1);\n \txdl_free_script(xscr2);\n \n-- \ngitgitgadget\n\n"},{"id":"448040","messageId":"b8f88f1b9f84fb3f8d71a75c49f33e3426f103c0.1644404356.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.git.1644404356.gitgitgadget@gmail.com","subject":"[PATCH 1/3] xdiff: handle allocation failure in patience diff","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T10:59:13Z","receivedAt":"2022-02-09T11:58:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nOther users of libxdiff such as libgit2 need to be able to handle\nallocation failures. As NULL is a valid return value the function\nsignature is changed to be able report allocation failures.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xpatience.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex c5d48e80aef..3e3d99f8922 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -198,7 +198,7 @@ static int binary_search(struct entry **sequence, int longest,\n  * item per sequence length: the sequence with the smallest last\n  * element (in terms of line2).\n  */\n-static struct entry *find_longest_common_sequence(struct hashmap *map)\n+static int find_longest_common_sequence(struct hashmap *map, struct entry **res)\n {\n \tstruct entry **sequence = xdl_malloc(map->nr * sizeof(struct entry *));\n \tint longest = 0, i;\n@@ -211,6 +211,9 @@ static struct entry *find_longest_common_sequence(struct hashmap *map)\n \t */\n \tint anchor_i = -1;\n \n+\tif (!sequence)\n+\t\treturn -1;\n+\n \tfor (entry = map->first; entry; entry = entry->next) {\n \t\tif (!entry->line2 || entry->line2 == NON_UNIQUE)\n \t\t\tcontinue;\n@@ -230,8 +233,9 @@ static struct entry *find_longest_common_sequence(struct hashmap *map)\n \n \t/* No common unique lines were found */\n \tif (!longest) {\n+\t\t*res = NULL;\n \t\txdl_free(sequence);\n-\t\treturn NULL;\n+\t\treturn 0;\n \t}\n \n \t/* Iterate starting at the last element, adjusting the \"next\" members */\n@@ -241,8 +245,9 @@ static struct entry *find_longest_common_sequence(struct hashmap *map)\n \t\tentry->previous->next = entry;\n \t\tentry = entry->previous;\n \t}\n+\t*res = entry;\n \txdl_free(sequence);\n-\treturn entry;\n+\treturn 0;\n }\n \n static int match(struct hashmap *map, int line1, int line2)\n@@ -358,14 +363,16 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n \t\treturn 0;\n \t}\n \n-\tfirst = find_longest_common_sequence(&map);\n+\tresult = find_longest_common_sequence(&map, &first);\n+\tif (result)\n+\t\tgoto out;\n \tif (first)\n \t\tresult = walk_common_sequence(&map, first,\n \t\t\tline1, count1, line2, count2);\n \telse\n \t\tresult = fall_back_to_classic_diff(&map,\n \t\t\tline1, count1, line2, count2);\n-\n+ out:\n \txdl_free(map.entries);\n \treturn result;\n }\n-- \ngitgitgadget\n\n"},{"id":"448061","messageId":"xmqqmtiz9aro.fsf@gitster.g","threadId":"57391","inReplyTo":"8655bb0348d7344ae85c8d521fb1ec7a5f4188d2.1644404356.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] xdiff: refactor a function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-09T18:04:43Z","receivedAt":"2022-02-09T18:04:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Rather than having to remember exactly what to free after an\n> allocation failure use the standard \"goto out\" pattern. This will\n> simplify the next commit that starts handling currently unhandled\n> failures.\n\nIt sound like this is meant to be a no-op clean-up; let's see.\n\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  xdiff/xmerge.c | 36 ++++++++++++++++--------------------\n>  1 file changed, 16 insertions(+), 20 deletions(-)\n>\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index fff0b594f9a..48c5e9e3f35 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n>  int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>  \t\txmparam_t const *xmp, mmbuffer_t *result)\n>  {\n> -\txdchange_t *xscr1, *xscr2;\n> -\txdfenv_t xe1, xe2;\n> -\tint status;\n> +\txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n> +\txdfenv_t xe1 = { 0 }, xe2 = { 0 };\n> +\tint status = -1;\n>  \txpparam_t const *xpp = &xmp->xpp;\n>  \n>  \tresult->ptr = NULL;\n>  \tresult->size = 0;\n>  \n> -\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n> -\t\treturn -1;\n> -\t}\n> -\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n> -\t\txdl_free_env(&xe1);\n> -\t\treturn -1;\n> -\t}\n\nOK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I\ndidn't check if patience and histogram also do so correctly), so the\noriginal was not leaking xe1 or xe2.\n\n> +\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0)\n> +\t\tgoto out;\n> +\n> +\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0)\n> +\t\tgoto out;\n> +\n\nAnd this does not change that.\n\n>  \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n>  \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n> -\t    xdl_build_script(&xe1, &xscr1) < 0) {\n> -\t\txdl_free_env(&xe1);\n> -\t\treturn -1;\n> -\t}\n> +\t    xdl_build_script(&xe1, &xscr1) < 0)\n> +\t\tgoto out;\n> +\n\nHere, as xdl_build_script() does free the script it failed to build,\nbut not the xe it was given, the original is correct to free xe1.\n\nWe jump from here to leave the freeing of xe1 to the clean-up part.\n\n>  \tif (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||\n>  \t    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||\n> -\t    xdl_build_script(&xe2, &xscr2) < 0) {\n> -\t\txdl_free_script(xscr1);\n> -\t\txdl_free_env(&xe1);\n> -\t\txdl_free_env(&xe2);\n> -\t\treturn -1;\n> -\t}\n> +\t    xdl_build_script(&xe2, &xscr2) < 0)\n> +\t\tgoto out;\n\nDitto for xe2 here.\n\n>  \tstatus = 0;\n>  \tif (!xscr1) {\n>  \t\tresult->ptr = xdl_malloc(mf2->size);\n> @@ -727,6 +722,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>  \t\t\t\t      &xe2, xscr2,\n>  \t\t\t\t      xmp, result);\n>  \t}\n> + out:\n>  \txdl_free_script(xscr1);\n>  \txdl_free_script(xscr2);\n\nAnd after the post-context of this hunk, we do free_env() both xe1\nand xe2, so we should be doing the same thing as before.\n\nLooking good.\n\n"},{"id":"448100","messageId":"xmqq8rujxmkf.fsf@gitster.g","threadId":"57391","inReplyTo":"xmqqmtiz9aro.fsf@gitster.g","subject":"Re: [PATCH 2/3] xdiff: refactor a function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-10T06:28:16Z","receivedAt":"2022-02-10T06:28:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>  int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>>  \t\txmparam_t const *xmp, mmbuffer_t *result)\n>>  {\n>> -\txdchange_t *xscr1, *xscr2;\n>> -\txdfenv_t xe1, xe2;\n>> -\tint status;\n>> +\txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n>> +\txdfenv_t xe1 = { 0 }, xe2 = { 0 };\n>> +\tint status = -1;\n>>  \txpparam_t const *xpp = &xmp->xpp;\n>>  \n>>  \tresult->ptr = NULL;\n>>  \tresult->size = 0;\n>>  \n>> -\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n>> -\t\treturn -1;\n>> -\t}\n>> -\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n>> -\t\txdl_free_env(&xe1);\n>> -\t\treturn -1;\n>> -\t}\n>\n> OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I\n> didn't check if patience and histogram also do so correctly), so the\n> original was not leaking xe1 or xe2.\n\nAfter I wrote the above, I took a brief look at patience and histogram,\nand it does not seem to release resources held by \"env\" when it signals\na failure by returning a negative value.  So it seems that the original\nused with patience or histogram were leaking env when it failed, and\nthis patch plugs that small leak.\n\nIf that is indeed the case, please note it in the proposed log\nmessage.\n\nThanks.\n"},{"id":"448221","messageId":"9c5b3acb-aabc-3a0d-f4e7-e10cec410dbe@gmail.com","threadId":"57391","inReplyTo":"xmqq8rujxmkf.fsf@gitster.g","subject":"Re: [PATCH 2/3] xdiff: refactor a function","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-11T15:19:29Z","receivedAt":"2022-02-11T15:19:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 10/02/2022 06:28, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>>>   int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>>>   \t\txmparam_t const *xmp, mmbuffer_t *result)\n>>>   {\n>>> -\txdchange_t *xscr1, *xscr2;\n>>> -\txdfenv_t xe1, xe2;\n>>> -\tint status;\n>>> +\txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n>>> +\txdfenv_t xe1 = { 0 }, xe2 = { 0 };\n>>> +\tint status = -1;\n>>>   \txpparam_t const *xpp = &xmp->xpp;\n>>>   \n>>>   \tresult->ptr = NULL;\n>>>   \tresult->size = 0;\n>>>   \n>>> -\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n>>> -\t\treturn -1;\n>>> -\t}\n>>> -\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n>>> -\t\txdl_free_env(&xe1);\n>>> -\t\treturn -1;\n>>> -\t}\n>>\n>> OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I\n>> didn't check if patience and histogram also do so correctly), so the\n>> original was not leaking xe1 or xe2.\n> \n> After I wrote the above, I took a brief look at patience and histogram,\n> and it does not seem to release resources held by \"env\" when it signals\n> a failure by returning a negative value.  So it seems that the original\n> used with patience or histogram were leaking env when it failed, and\n> this patch plugs that small leak.\n> \n> If that is indeed the case, please note it in the proposed log\n> message.\n\nOh well spotted, xdl_do_diff() only frees \"env\" if the myers algorithm \nhas an error, if the patience or histogram algorithms have an error then \nthey do not free \"env\" and it is not freed by xdl_do_diff(). This patch \ninadvertently fixes that leak when merging but not when calling \nxdl_do_diff() to compact conflicts in zealous mode or when doing a plain \ndiff. I think the simplest fix is to have xdl_do_diff() free \"env\" when \nthere is an error what ever algorithm is used.\n\nI'll try to put a patch together to fix the other cases. If we fix this \nleak in xdl_do_diff() then maybe we should go back to returning -1 in \nthe hunk above and explain in the log message why that is ok.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n"},{"id":"448226","messageId":"xmqqleyhe4gp.fsf@gitster.g","threadId":"57391","inReplyTo":"9c5b3acb-aabc-3a0d-f4e7-e10cec410dbe@gmail.com","subject":"Re: [PATCH 2/3] xdiff: refactor a function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T16:46:30Z","receivedAt":"2022-02-11T16:46:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Oh well spotted, xdl_do_diff() only frees \"env\" if the myers algorithm\n> has an error, if the patience or histogram algorithms have an error\n> then they do not free \"env\" and it is not freed by xdl_do_diff(). This\n> patch inadvertently fixes that leak when merging but not when calling \n> xdl_do_diff() to compact conflicts in zealous mode or when doing a\n> plain diff. I think the simplest fix is to have xdl_do_diff() free\n> \"env\" when there is an error what ever algorithm is used.\n\nHeh, I didn't even look at the other uses of xdl_do_diff(); I am\nglad you did.\n\n> I'll try to put a patch together to fix the other cases. If we fix\n> this leak in xdl_do_diff() then maybe we should go back to returning\n> -1 in the hunk above and explain in the log message why that is ok.\n\nThanks.\n"},{"id":"448570","messageId":"pull.1140.v2.git.1645006510.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.git.1644404356.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] xdiff: handle allocation failures","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:15:05Z","receivedAt":"2022-02-16T10:15:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks to Junio for his comments on V1.\n\nChanges since V1:\n\n * Patch 1 is new and addresses a memory leak noticed by Junio\n * Patch 2 is unchanged\n * Patch 3 now avoids a double free of xe1 on error\n * Patch 4 reworked handling of the return value\n\nV1 Cover Letter: Other users of xdiff such as libgit2 need to be able to\nhandle allocation failures. This series fixes the few cases where we are not\ndoing this already.\n\nEdward's patch[1] reminded me that I had these waiting to be submitted.\n\n[1] https://lore.kernel.org/git/20220209013354.GB7@abe733c6e288/\n\nPhillip Wood (4):\n  xdiff: fix a memory leak\n  xdiff: handle allocation failure in patience diff\n  xdiff: refactor a function\n  xdiff: handle allocation failure when merging\n\n xdiff/xdiffi.c     | 33 +++++++++++++++++----------------\n xdiff/xhistogram.c |  3 ---\n xdiff/xmerge.c     | 42 ++++++++++++++++++++++--------------------\n xdiff/xpatience.c  | 21 ++++++++++++---------\n 4 files changed, 51 insertions(+), 48 deletions(-)\n\n\nbase-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1140%2Fphillipwood%2Fwip%2Fxdiff-handle-allocation-failures-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1140/phillipwood/wip/xdiff-handle-allocation-failures-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1140\n\nRange-diff vs v1:\n\n -:  ----------- > 1:  3fcb6c80367 xdiff: fix a memory leak\n 1:  b8f88f1b9f8 = 2:  fec1f4a4152 xdiff: handle allocation failure in patience diff\n 2:  8655bb0348d ! 3:  073a45e9e85 xdiff: refactor a function\n     @@ Metadata\n       ## Commit message ##\n          xdiff: refactor a function\n      \n     -    Rather than having to remember exactly what to free after an\n     -    allocation failure use the standard \"goto out\" pattern. This will\n     -    simplify the next commit that starts handling currently unhandled\n     -    failures.\n     +    Use the standard \"goto out\" pattern rather than repeating very similar\n     +    code after checking for each error. This will simplify the next commit\n     +    that starts handling allocation failures that are currently ignored.\n     +    On error xdl_do_diff() frees the environment so we need to take care\n     +    to avoid a double free in that case. xdl_build_script() does not\n     +    assign a result unless it is successful so there is no possibility of\n     +    a double free if it fails.\n      \n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n     @@ xdiff/xmerge.c: static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n       \t\txmparam_t const *xmp, mmbuffer_t *result)\n       {\n      -\txdchange_t *xscr1, *xscr2;\n     --\txdfenv_t xe1, xe2;\n     --\tint status;\n      +\txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n     -+\txdfenv_t xe1 = { 0 }, xe2 = { 0 };\n     + \txdfenv_t xe1, xe2;\n     +-\tint status;\n      +\tint status = -1;\n       \txpparam_t const *xpp = &xmp->xpp;\n       \n     @@ xdiff/xmerge.c: static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n       \tresult->size = 0;\n       \n      -\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n     --\t\treturn -1;\n     ++\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0)\n     + \t\treturn -1;\n      -\t}\n      -\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n      -\t\txdl_free_env(&xe1);\n      -\t\treturn -1;\n      -\t}\n     -+\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0)\n     -+\t\tgoto out;\n      +\n      +\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0)\n     -+\t\tgoto out;\n     ++\t\tgoto free_xe1; /* avoid double free of xe2 */\n      +\n       \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n       \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n     @@ xdiff/xmerge.c: int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n       \txdl_free_script(xscr1);\n       \txdl_free_script(xscr2);\n       \n     +-\txdl_free_env(&xe1);\n     + \txdl_free_env(&xe2);\n     ++ free_xe1:\n     ++\txdl_free_env(&xe1);\n     + \n     + \treturn status;\n     + }\n 3:  3e935abba1d ! 4:  7e705d771b0 xdiff: handle allocation failure when merging\n     @@ Commit message\n      \n       ## xdiff/xmerge.c ##\n      @@ xdiff/xmerge.c: int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n     - \tstatus = 0;\n     + \t    xdl_build_script(&xe2, &xscr2) < 0)\n     + \t\tgoto out;\n     + \n     +-\tstatus = 0;\n       \tif (!xscr1) {\n       \t\tresult->ptr = xdl_malloc(mf2->size);\n     -+\t\tif (!result->ptr) {\n     -+\t\t\tstatus = -1;\n     ++\t\tif (!result->ptr)\n      +\t\t\tgoto out;\n     -+\t\t}\n     ++\t\tstatus = 0;\n       \t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n       \t\tresult->size = mf2->size;\n       \t} else if (!xscr2) {\n       \t\tresult->ptr = xdl_malloc(mf1->size);\n     -+\t\tif (!result->ptr) {\n     -+\t\t\tstatus = -1;\n     ++\t\tif (!result->ptr)\n      +\t\t\tgoto out;\n     -+\t\t}\n     ++\t\tstatus = 0;\n       \t\tmemcpy(result->ptr, mf1->ptr, mf1->size);\n       \t\tresult->size = mf1->size;\n       \t} else {\n\n-- \ngitgitgadget\n"},{"id":"448571","messageId":"3fcb6c8036794a16c4aa9285e3ef70da02e2bc80.1645006510.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.v2.git.1645006510.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] xdiff: fix a memory leak","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:15:06Z","receivedAt":"2022-02-16T10:15:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAlthough the patience and histogram algorithms initialize the\nenvironment they do not free it if there is an error. In contrast for\nthe Myers algorithm the environment is initalized in xdl_do_diff() and\nit is freed if there is an error. Fix this by always initializing the\nenvironment in xdl_do_diff() and freeing it there if there is an\nerror. Remove the comment in do_patience_diff() about the environment\nbeing freed by xdl_diff() as it is not accurate because (a) xdl_diff()\ndoes not do that if there is an error and (b) xdl_diff() is not the\nonly caller.\n\nReported-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xdiffi.c     | 33 +++++++++++++++++----------------\n xdiff/xhistogram.c |  3 ---\n xdiff/xpatience.c  |  4 ----\n 3 files changed, 17 insertions(+), 23 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 69689fab247..758410c11ac 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -315,16 +315,19 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \tlong *kvd, *kvdf, *kvdb;\n \txdalgoenv_t xenv;\n \tdiffdata_t dd1, dd2;\n+\tint res;\n \n-\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF)\n-\t\treturn xdl_do_patience_diff(mf1, mf2, xpp, xe);\n-\n-\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)\n-\t\treturn xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n+\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)\n+\t\treturn -1;\n \n-\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0) {\n+\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {\n+\t\tres = xdl_do_patience_diff(mf1, mf2, xpp, xe);\n+\t\tgoto out;\n+\t}\n \n-\t\treturn -1;\n+\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {\n+\t\tres = xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n+\t\tgoto out;\n \t}\n \n \t/*\n@@ -359,17 +362,15 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \tdd2.rchg = xe->xdf2.rchg;\n \tdd2.rindex = xe->xdf2.rindex;\n \n-\tif (xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n-\t\t\t kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0, &xenv) < 0) {\n-\n-\t\txdl_free(kvd);\n-\t\txdl_free_env(xe);\n-\t\treturn -1;\n-\t}\n-\n+\tres = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n+\t\t\t   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,\n+\t\t\t   &xenv);\n \txdl_free(kvd);\n+ out:\n+\tif (res < 0)\n+\t\txdl_free_env(xe);\n \n-\treturn 0;\n+\treturn res;\n }\n \n \ndiff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\nindex 80794748b0d..01decffc332 100644\n--- a/xdiff/xhistogram.c\n+++ b/xdiff/xhistogram.c\n@@ -372,9 +372,6 @@ out:\n int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2,\n \txpparam_t const *xpp, xdfenv_t *env)\n {\n-\tif (xdl_prepare_env(file1, file2, xpp, env) < 0)\n-\t\treturn -1;\n-\n \treturn histogram_diff(xpp, env,\n \t\tenv->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1,\n \t\tenv->xdf2.dstart + 1, env->xdf2.dend - env->xdf2.dstart + 1);\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex c5d48e80aef..e8de8d150ce 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -373,10 +373,6 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2,\n \t\txpparam_t const *xpp, xdfenv_t *env)\n {\n-\tif (xdl_prepare_env(file1, file2, xpp, env) < 0)\n-\t\treturn -1;\n-\n-\t/* environment is cleaned up in xdl_diff() */\n \treturn patience_diff(file1, file2, xpp, env,\n \t\t\t1, env->xdf1.nrec, 1, env->xdf2.nrec);\n }\n-- \ngitgitgadget\n\n"},{"id":"448572","messageId":"fec1f4a4152dbde8909ad640bf7adbd7622a64da.1645006510.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.v2.git.1645006510.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] xdiff: handle allocation failure in patience diff","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:15:07Z","receivedAt":"2022-02-16T10:15:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nOther users of libxdiff such as libgit2 need to be able to handle\nallocation failures. As NULL is a valid return value the function\nsignature is changed to be able report allocation failures.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xpatience.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex e8de8d150ce..1a21c6a74b3 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -198,7 +198,7 @@ static int binary_search(struct entry **sequence, int longest,\n  * item per sequence length: the sequence with the smallest last\n  * element (in terms of line2).\n  */\n-static struct entry *find_longest_common_sequence(struct hashmap *map)\n+static int find_longest_common_sequence(struct hashmap *map, struct entry **res)\n {\n \tstruct entry **sequence = xdl_malloc(map->nr * sizeof(struct entry *));\n \tint longest = 0, i;\n@@ -211,6 +211,9 @@ static struct entry *find_longest_common_sequence(struct hashmap *map)\n \t */\n \tint anchor_i = -1;\n \n+\tif (!sequence)\n+\t\treturn -1;\n+\n \tfor (entry = map->first; entry; entry = entry->next) {\n \t\tif (!entry->line2 || entry->line2 == NON_UNIQUE)\n \t\t\tcontinue;\n@@ -230,8 +233,9 @@ static struct entry *find_longest_common_sequence(struct hashmap *map)\n \n \t/* No common unique lines were found */\n \tif (!longest) {\n+\t\t*res = NULL;\n \t\txdl_free(sequence);\n-\t\treturn NULL;\n+\t\treturn 0;\n \t}\n \n \t/* Iterate starting at the last element, adjusting the \"next\" members */\n@@ -241,8 +245,9 @@ static struct entry *find_longest_common_sequence(struct hashmap *map)\n \t\tentry->previous->next = entry;\n \t\tentry = entry->previous;\n \t}\n+\t*res = entry;\n \txdl_free(sequence);\n-\treturn entry;\n+\treturn 0;\n }\n \n static int match(struct hashmap *map, int line1, int line2)\n@@ -358,14 +363,16 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n \t\treturn 0;\n \t}\n \n-\tfirst = find_longest_common_sequence(&map);\n+\tresult = find_longest_common_sequence(&map, &first);\n+\tif (result)\n+\t\tgoto out;\n \tif (first)\n \t\tresult = walk_common_sequence(&map, first,\n \t\t\tline1, count1, line2, count2);\n \telse\n \t\tresult = fall_back_to_classic_diff(&map,\n \t\t\tline1, count1, line2, count2);\n-\n+ out:\n \txdl_free(map.entries);\n \treturn result;\n }\n-- \ngitgitgadget\n\n"},{"id":"448573","messageId":"073a45e9e8563686b43cb4f9beb4295523fb797f.1645006510.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.v2.git.1645006510.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] xdiff: refactor a function","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:15:08Z","receivedAt":"2022-02-16T10:15:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nUse the standard \"goto out\" pattern rather than repeating very similar\ncode after checking for each error. This will simplify the next commit\nthat starts handling allocation failures that are currently ignored.\nOn error xdl_do_diff() frees the environment so we need to take care\nto avoid a double free in that case. xdl_build_script() does not\nassign a result unless it is successful so there is no possibility of\na double free if it fails.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xmerge.c | 35 ++++++++++++++++-------------------\n 1 file changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex fff0b594f9a..d43abe05b95 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t\txmparam_t const *xmp, mmbuffer_t *result)\n {\n-\txdchange_t *xscr1, *xscr2;\n+\txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n \txdfenv_t xe1, xe2;\n-\tint status;\n+\tint status = -1;\n \txpparam_t const *xpp = &xmp->xpp;\n \n \tresult->ptr = NULL;\n \tresult->size = 0;\n \n-\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n+\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0)\n \t\treturn -1;\n-\t}\n-\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n-\t\txdl_free_env(&xe1);\n-\t\treturn -1;\n-\t}\n+\n+\tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0)\n+\t\tgoto free_xe1; /* avoid double free of xe2 */\n+\n \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n-\t    xdl_build_script(&xe1, &xscr1) < 0) {\n-\t\txdl_free_env(&xe1);\n-\t\treturn -1;\n-\t}\n+\t    xdl_build_script(&xe1, &xscr1) < 0)\n+\t\tgoto out;\n+\n \tif (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||\n-\t    xdl_build_script(&xe2, &xscr2) < 0) {\n-\t\txdl_free_script(xscr1);\n-\t\txdl_free_env(&xe1);\n-\t\txdl_free_env(&xe2);\n-\t\treturn -1;\n-\t}\n+\t    xdl_build_script(&xe2, &xscr2) < 0)\n+\t\tgoto out;\n+\n \tstatus = 0;\n \tif (!xscr1) {\n \t\tresult->ptr = xdl_malloc(mf2->size);\n@@ -727,11 +722,13 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t\t\t\t      &xe2, xscr2,\n \t\t\t\t      xmp, result);\n \t}\n+ out:\n \txdl_free_script(xscr1);\n \txdl_free_script(xscr2);\n \n-\txdl_free_env(&xe1);\n \txdl_free_env(&xe2);\n+ free_xe1:\n+\txdl_free_env(&xe1);\n \n \treturn status;\n }\n-- \ngitgitgadget\n\n"},{"id":"448574","messageId":"7e705d771b041ae0d455a4296e2955a24e264779.1645006510.git.gitgitgadget@gmail.com","threadId":"57391","inReplyTo":"pull.1140.v2.git.1645006510.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] xdiff: handle allocation failure when merging","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T10:15:09Z","receivedAt":"2022-02-16T10:15:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nOther users of xdiff such as libgit2 need to be able to handle\nallocation failures. These allocation failures were previously\nignored.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff/xmerge.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex d43abe05b95..af40c88a5b3 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -708,13 +708,18 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t    xdl_build_script(&xe2, &xscr2) < 0)\n \t\tgoto out;\n \n-\tstatus = 0;\n \tif (!xscr1) {\n \t\tresult->ptr = xdl_malloc(mf2->size);\n+\t\tif (!result->ptr)\n+\t\t\tgoto out;\n+\t\tstatus = 0;\n \t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n \t\tresult->size = mf2->size;\n \t} else if (!xscr2) {\n \t\tresult->ptr = xdl_malloc(mf1->size);\n+\t\tif (!result->ptr)\n+\t\t\tgoto out;\n+\t\tstatus = 0;\n \t\tmemcpy(result->ptr, mf1->ptr, mf1->size);\n \t\tresult->size = mf1->size;\n \t} else {\n-- \ngitgitgadget\n"},{"id":"448577","messageId":"220216.86tuczt7js.gmgdl@evledraar.gmail.com","threadId":"57391","inReplyTo":"3fcb6c8036794a16c4aa9285e3ef70da02e2bc80.1645006510.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] xdiff: fix a memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T10:28:56Z","receivedAt":"2022-02-16T10:44:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 16 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Although the patience and histogram algorithms initialize the\n> environment they do not free it if there is an error. In contrast for\n> the Myers algorithm the environment is initalized in xdl_do_diff() and\n> it is freed if there is an error. Fix this by always initializing the\n> environment in xdl_do_diff() and freeing it there if there is an\n> error. Remove the comment in do_patience_diff() about the environment\n> being freed by xdl_diff() as it is not accurate because (a) xdl_diff()\n> does not do that if there is an error and (b) xdl_diff() is not the\n> only caller.\n>\n> Reported-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  xdiff/xdiffi.c     | 33 +++++++++++++++++----------------\n>  xdiff/xhistogram.c |  3 ---\n>  xdiff/xpatience.c  |  4 ----\n>  3 files changed, 17 insertions(+), 23 deletions(-)\n>\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 69689fab247..758410c11ac 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -315,16 +315,19 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>  \tlong *kvd, *kvdf, *kvdb;\n>  \txdalgoenv_t xenv;\n>  \tdiffdata_t dd1, dd2;\n> +\tint res;\n>  \n> -\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF)\n> -\t\treturn xdl_do_patience_diff(mf1, mf2, xpp, xe);\n> -\n> -\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)\n> -\t\treturn xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n> +\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)\n> +\t\treturn -1;\n>  \n> -\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0) {\n> +\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {\n> +\t\tres = xdl_do_patience_diff(mf1, mf2, xpp, xe);\n> +\t\tgoto out;\n> +\t}\n>  \n> -\t\treturn -1;\n> +\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {\n> +\t\tres = xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n> +\t\tgoto out;\n>  \t}\n>  \n>  \t/*\n> @@ -359,17 +362,15 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>  \tdd2.rchg = xe->xdf2.rchg;\n>  \tdd2.rindex = xe->xdf2.rindex;\n>  \n> -\tif (xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n> -\t\t\t kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0, &xenv) < 0) {\n> -\n> -\t\txdl_free(kvd);\n> -\t\txdl_free_env(xe);\n> -\t\treturn -1;\n> -\t}\n> -\n> +\tres = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n> +\t\t\t   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,\n> +\t\t\t   &xenv);\n>  \txdl_free(kvd);\n> + out:\n> +\tif (res < 0)\n> +\t\txdl_free_env(xe);\n>  \n> -\treturn 0;\n> +\treturn res;\n>  }\n\nI wanted to test if this made some diff test pass under SANITIZE=leak\nthat didn't before, and my usual quick way to discover that is to run\nthe tests with something like this for testing:\n\t\n\tdiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n\tindex 758410c11ac..7811ce2a1d3 100644\n\t--- a/xdiff/xdiffi.c\n\t+++ b/xdiff/xdiffi.c\n\t@@ -368,7 +368,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n\t \txdl_free(kvd);\n\t  out:\n\t \tif (res < 0)\n\t-\t\txdl_free_env(xe);\n\t+\t\tBUG(\"needed freeing\");\n\t \n\t \treturn res;\n\t }\n\nBut doing so has all tests passing, so either this code is unreachable\nor not reachable by any of our tests.\n\nMore generally I think this patch is taking the wrong approach, why are\nwe trying to the members of a stack variable in xdl_do_diff(), when that\nvariable isn't ours, but is created by our caller? Why aren't they\ndealing with it?\n\nThe main issue the memory handling is such a pain here is because of\nthat mixed ownership.\n\nThe below POC (and seems to pass tests) shows a way to do that, which I\nthink in general is *much* simpler. I.e. sure we'll call free()\nredundantly some of the time, but that'll be safe as long as we\nzero-initialize the relevant struct.\n\nThe xdiff code is much harder to read & maintain than it needs to be\nbecause it's trying to micro-optimize these sort of patterns.\n\nE.g. with the diff below we'll now redundantly call the free on a xe2\nwhen we only used a xe1, but \"who cares?\" is my considered opinion on\nthat topic :)\n\nBy not trying to micro-optimize it like that the memory management and\nownership becomes so much easier to deal with.\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 758410c11ac..6ad30a98b62 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -1054,19 +1054,19 @@ static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t     xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n \txdchange_t *xscr;\n-\txdfenv_t xe;\n+\txdfenv_t xe = { 0 };\n \temit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;\n+\tint status = 0;\n \n \tif (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {\n-\n-\t\treturn -1;\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n \t}\n \tif (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||\n \t    xdl_build_script(&xe, &xscr) < 0) {\n-\n-\t\txdl_free_env(&xe);\n-\t\treturn -1;\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n \t}\n \tif (xscr) {\n \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n@@ -1078,12 +1078,14 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n \n \t\t\txdl_free_script(xscr);\n-\t\t\txdl_free_env(&xe);\n-\t\t\treturn -1;\n+\t\t\tstatus = -1;\n+\t\t\tgoto cleanup;\n \t\t}\n \t\txdl_free_script(xscr);\n \t}\n+\n+cleanup:\n \txdl_free_env(&xe);\n \n-\treturn 0;\n+\treturn status;\n }\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex fff0b594f9a..4751eab9c12 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -685,7 +685,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t\txmparam_t const *xmp, mmbuffer_t *result)\n {\n \txdchange_t *xscr1, *xscr2;\n-\txdfenv_t xe1, xe2;\n+\txdfenv_t xe1 = { 0 };\n+\txdfenv_t xe2 = { 0 };\n \tint status;\n \txpparam_t const *xpp = &xmp->xpp;\n \n@@ -693,25 +694,25 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \tresult->size = 0;\n \n \tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n-\t\treturn -1;\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n \t}\n \tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n-\t\txdl_free_env(&xe1);\n-\t\treturn -1;\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n \t}\n \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n \t    xdl_build_script(&xe1, &xscr1) < 0) {\n-\t\txdl_free_env(&xe1);\n-\t\treturn -1;\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n \t}\n \tif (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||\n \t    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||\n \t    xdl_build_script(&xe2, &xscr2) < 0) {\n \t\txdl_free_script(xscr1);\n-\t\txdl_free_env(&xe1);\n-\t\txdl_free_env(&xe2);\n-\t\treturn -1;\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n \t}\n \tstatus = 0;\n \tif (!xscr1) {\n@@ -730,6 +731,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \txdl_free_script(xscr1);\n \txdl_free_script(xscr2);\n \n+cleanup:\n \txdl_free_env(&xe1);\n \txdl_free_env(&xe2);\n \ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex cfa6e2220ff..63fb2eee975 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -414,7 +414,8 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,\n \t * ranges of lines instead of the whole files.\n \t */\n \tmmfile_t subfile1, subfile2;\n-\txdfenv_t env;\n+\txdfenv_t env = { 0 };\n+\tint status = 0;\n \n \tsubfile1.ptr = (char *)diff_env->xdf1.recs[line1 - 1]->ptr;\n \tsubfile1.size = diff_env->xdf1.recs[line1 + count1 - 2]->ptr +\n@@ -422,13 +423,16 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,\n \tsubfile2.ptr = (char *)diff_env->xdf2.recs[line2 - 1]->ptr;\n \tsubfile2.size = diff_env->xdf2.recs[line2 + count2 - 2]->ptr +\n \t\tdiff_env->xdf2.recs[line2 + count2 - 2]->size - subfile2.ptr;\n-\tif (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0)\n-\t\treturn -1;\n+\tif (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0) {\n+\t\tstatus = -1;\n+\t\tgoto cleanup;\n+\t}\n \n \tmemcpy(diff_env->xdf1.rchg + line1 - 1, env.xdf1.rchg, count1);\n \tmemcpy(diff_env->xdf2.rchg + line2 - 1, env.xdf2.rchg, count2);\n \n+cleanup:\n \txdl_free_env(&env);\n \n-\treturn 0;\n+\treturn status;\n }\n"},{"id":"448591","messageId":"10c8dfa8-ab98-4a46-8baa-97857bb914b0@gmail.com","threadId":"57391","inReplyTo":"220216.86tuczt7js.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/4] xdiff: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-16T13:49:31Z","receivedAt":"2022-02-16T13:49:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nThanks for taking a look at this patch\n\nOn 16/02/2022 10:28, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Feb 16 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Although the patience and histogram algorithms initialize the\n>> environment they do not free it if there is an error. In contrast for\n>> the Myers algorithm the environment is initalized in xdl_do_diff() and\n>> it is freed if there is an error. Fix this by always initializing the\n>> environment in xdl_do_diff() and freeing it there if there is an\n>> error. Remove the comment in do_patience_diff() about the environment\n>> being freed by xdl_diff() as it is not accurate because (a) xdl_diff()\n>> does not do that if there is an error and (b) xdl_diff() is not the\n>> only caller.\n>>\n>> Reported-by: Junio C Hamano <gitster@pobox.com>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>   xdiff/xdiffi.c     | 33 +++++++++++++++++----------------\n>>   xdiff/xhistogram.c |  3 ---\n>>   xdiff/xpatience.c  |  4 ----\n>>   3 files changed, 17 insertions(+), 23 deletions(-)\n>>\n>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>> index 69689fab247..758410c11ac 100644\n>> --- a/xdiff/xdiffi.c\n>> +++ b/xdiff/xdiffi.c\n>> @@ -315,16 +315,19 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>>   \tlong *kvd, *kvdf, *kvdb;\n>>   \txdalgoenv_t xenv;\n>>   \tdiffdata_t dd1, dd2;\n>> +\tint res;\n>>   \n>> -\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF)\n>> -\t\treturn xdl_do_patience_diff(mf1, mf2, xpp, xe);\n>> -\n>> -\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)\n>> -\t\treturn xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n>> +\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)\n>> +\t\treturn -1;\n>>   \n>> -\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0) {\n>> +\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {\n>> +\t\tres = xdl_do_patience_diff(mf1, mf2, xpp, xe);\n>> +\t\tgoto out;\n>> +\t}\n>>   \n>> -\t\treturn -1;\n>> +\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {\n>> +\t\tres = xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n>> +\t\tgoto out;\n>>   \t}\n>>   \n>>   \t/*\n>> @@ -359,17 +362,15 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>>   \tdd2.rchg = xe->xdf2.rchg;\n>>   \tdd2.rindex = xe->xdf2.rindex;\n>>   \n>> -\tif (xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n>> -\t\t\t kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0, &xenv) < 0) {\n>> -\n>> -\t\txdl_free(kvd);\n>> -\t\txdl_free_env(xe);\n>> -\t\treturn -1;\n>> -\t}\n>> -\n>> +\tres = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n>> +\t\t\t   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,\n>> +\t\t\t   &xenv);\n>>   \txdl_free(kvd);\n>> + out:\n>> +\tif (res < 0)\n>> +\t\txdl_free_env(xe);\n>>   \n>> -\treturn 0;\n>> +\treturn res;\n>>   }\n> \n> I wanted to test if this made some diff test pass under SANITIZE=leak\n> that didn't before, and my usual quick way to discover that is to run\n> the tests with something like this for testing:\n> \t\n> \tdiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> \tindex 758410c11ac..7811ce2a1d3 100644\n> \t--- a/xdiff/xdiffi.c\n> \t+++ b/xdiff/xdiffi.c\n> \t@@ -368,7 +368,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t \txdl_free(kvd);\n> \t  out:\n> \t \tif (res < 0)\n> \t-\t\txdl_free_env(xe);\n> \t+\t\tBUG(\"needed freeing\");\n> \t\n> \t \treturn res;\n> \t }\n> \n> But doing so has all tests passing, so either this code is unreachable\n> or not reachable by any of our tests.\n\nYeah, it is fixing a leak in the error path. The only possible error is \nan allocation failure and we die rather than returning in that case so \nthe test suite is not going to tell us anything about this patch.\n\n> More generally I think this patch is taking the wrong approach, why are\n> we trying to the members of a stack variable in xdl_do_diff(), when that\n> variable isn't ours, but is created by our caller? Why aren't they\n> dealing with it?\n\nThey are not dealing with it because they do not initialize it - it is \nan \"out\" parameter that is used to return data to the caller. This patch \nchanges the logic to \"whoever initializes it is responsible for freeing \nit if there is an error\". By doing that we localize the error handling \nto xdl_do_diff() and can leave the callers unchanged.\n\n> The main issue the memory handling is such a pain here is because of\n> that mixed ownership.\n> \n> The below POC (and seems to pass tests) shows a way to do that, which I\n> think in general is *much* simpler. I.e. sure we'll call free()\n> redundantly some of the time, but that'll be safe as long as we\n> zero-initialize the relevant struct.\n> \n> The xdiff code is much harder to read & maintain than it needs to be\n> because it's trying to micro-optimize these sort of patterns.\n> \n> E.g. with the diff below we'll now redundantly call the free on a xe2\n> when we only used a xe1, but \"who cares?\" is my considered opinion on\n> that topic :)\n\nThen you should reread the cover letter and commit message and code \ncomment in patch 3 that all mention avoiding a double free in that case. \nIf you have any ideas how to make those clearer please let me know.\n\nIf xdl_do_diff(orig, mf2, xpp, &xe2) fails it frees any members of xe2 \nthat it allocated but it does not NULL them. If we then call \nxdl_free_env(&xe2) we will have a double free error.\n\nBest Wishes\n\nPhillip\n\n> By not trying to micro-optimize it like that the memory management and\n> ownership becomes so much easier to deal with.\n> \n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 758410c11ac..6ad30a98b62 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -1054,19 +1054,19 @@ static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n>   int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>   \t     xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n>   \txdchange_t *xscr;\n> -\txdfenv_t xe;\n> +\txdfenv_t xe = { 0 };\n>   \temit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;\n> +\tint status = 0;\n>   \n>   \tif (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {\n> -\n> -\t\treturn -1;\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n>   \t}\n>   \tif (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||\n>   \t    xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||\n>   \t    xdl_build_script(&xe, &xscr) < 0) {\n> -\n> -\t\txdl_free_env(&xe);\n> -\t\treturn -1;\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n>   \t}\n>   \tif (xscr) {\n>   \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n> @@ -1078,12 +1078,14 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>   \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n>   \n>   \t\t\txdl_free_script(xscr);\n> -\t\t\txdl_free_env(&xe);\n> -\t\t\treturn -1;\n> +\t\t\tstatus = -1;\n> +\t\t\tgoto cleanup;\n>   \t\t}\n>   \t\txdl_free_script(xscr);\n>   \t}\n> +\n> +cleanup:\n>   \txdl_free_env(&xe);\n>   \n> -\treturn 0;\n> +\treturn status;\n>   }\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index fff0b594f9a..4751eab9c12 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -685,7 +685,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>   \t\txmparam_t const *xmp, mmbuffer_t *result)\n>   {\n>   \txdchange_t *xscr1, *xscr2;\n> -\txdfenv_t xe1, xe2;\n> +\txdfenv_t xe1 = { 0 };\n> +\txdfenv_t xe2 = { 0 };\n>   \tint status;\n>   \txpparam_t const *xpp = &xmp->xpp;\n>   \n> @@ -693,25 +694,25 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>   \tresult->size = 0;\n>   \n>   \tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n> -\t\treturn -1;\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n>   \t}\n>   \tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n> -\t\txdl_free_env(&xe1);\n> -\t\treturn -1;\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n>   \t}\n>   \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n>   \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n>   \t    xdl_build_script(&xe1, &xscr1) < 0) {\n> -\t\txdl_free_env(&xe1);\n> -\t\treturn -1;\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n>   \t}\n>   \tif (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||\n>   \t    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||\n>   \t    xdl_build_script(&xe2, &xscr2) < 0) {\n>   \t\txdl_free_script(xscr1);\n> -\t\txdl_free_env(&xe1);\n> -\t\txdl_free_env(&xe2);\n> -\t\treturn -1;\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n>   \t}\n>   \tstatus = 0;\n>   \tif (!xscr1) {\n> @@ -730,6 +731,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>   \txdl_free_script(xscr1);\n>   \txdl_free_script(xscr2);\n>   \n> +cleanup:\n>   \txdl_free_env(&xe1);\n>   \txdl_free_env(&xe2);\n>   \n> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n> index cfa6e2220ff..63fb2eee975 100644\n> --- a/xdiff/xutils.c\n> +++ b/xdiff/xutils.c\n> @@ -414,7 +414,8 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,\n>   \t * ranges of lines instead of the whole files.\n>   \t */\n>   \tmmfile_t subfile1, subfile2;\n> -\txdfenv_t env;\n> +\txdfenv_t env = { 0 };\n> +\tint status = 0;\n>   \n>   \tsubfile1.ptr = (char *)diff_env->xdf1.recs[line1 - 1]->ptr;\n>   \tsubfile1.size = diff_env->xdf1.recs[line1 + count1 - 2]->ptr +\n> @@ -422,13 +423,16 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,\n>   \tsubfile2.ptr = (char *)diff_env->xdf2.recs[line2 - 1]->ptr;\n>   \tsubfile2.size = diff_env->xdf2.recs[line2 + count2 - 2]->ptr +\n>   \t\tdiff_env->xdf2.recs[line2 + count2 - 2]->size - subfile2.ptr;\n> -\tif (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0)\n> -\t\treturn -1;\n> +\tif (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0) {\n> +\t\tstatus = -1;\n> +\t\tgoto cleanup;\n> +\t}\n>   \n>   \tmemcpy(diff_env->xdf1.rchg + line1 - 1, env.xdf1.rchg, count1);\n>   \tmemcpy(diff_env->xdf2.rchg + line2 - 1, env.xdf2.rchg, count2);\n>   \n> +cleanup:\n>   \txdl_free_env(&env);\n>   \n> -\treturn 0;\n> +\treturn status;\n>   }\n\n"},{"id":"448597","messageId":"220216.86h78yuar0.gmgdl@evledraar.gmail.com","threadId":"57391","inReplyTo":"10c8dfa8-ab98-4a46-8baa-97857bb914b0@gmail.com","subject":"Re: [PATCH v2 1/4] xdiff: fix a memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T14:38:08Z","receivedAt":"2022-02-16T14:50:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 16 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> Thanks for taking a look at this patch\n>\n> On 16/02/2022 10:28, Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Feb 16 2022, Phillip Wood via GitGitGadget wrote:\n>> \n>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>\n>>> Although the patience and histogram algorithms initialize the\n>>> environment they do not free it if there is an error. In contrast for\n>>> the Myers algorithm the environment is initalized in xdl_do_diff() and\n>>> it is freed if there is an error. Fix this by always initializing the\n>>> environment in xdl_do_diff() and freeing it there if there is an\n>>> error. Remove the comment in do_patience_diff() about the environment\n>>> being freed by xdl_diff() as it is not accurate because (a) xdl_diff()\n>>> does not do that if there is an error and (b) xdl_diff() is not the\n>>> only caller.\n>>>\n>>> Reported-by: Junio C Hamano <gitster@pobox.com>\n>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>> ---\n>>>   xdiff/xdiffi.c     | 33 +++++++++++++++++----------------\n>>>   xdiff/xhistogram.c |  3 ---\n>>>   xdiff/xpatience.c  |  4 ----\n>>>   3 files changed, 17 insertions(+), 23 deletions(-)\n>>>\n>>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>>> index 69689fab247..758410c11ac 100644\n>>> --- a/xdiff/xdiffi.c\n>>> +++ b/xdiff/xdiffi.c\n>>> @@ -315,16 +315,19 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>>>   \tlong *kvd, *kvdf, *kvdb;\n>>>   \txdalgoenv_t xenv;\n>>>   \tdiffdata_t dd1, dd2;\n>>> +\tint res;\n>>>   -\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF)\n>>> -\t\treturn xdl_do_patience_diff(mf1, mf2, xpp, xe);\n>>> -\n>>> -\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)\n>>> -\t\treturn xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n>>> +\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)\n>>> +\t\treturn -1;\n>>>   -\tif (xdl_prepare_env(mf1, mf2, xpp, xe) < 0) {\n>>> +\tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {\n>>> +\t\tres = xdl_do_patience_diff(mf1, mf2, xpp, xe);\n>>> +\t\tgoto out;\n>>> +\t}\n>>>   -\t\treturn -1;\n>>> +\tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {\n>>> +\t\tres = xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n>>> +\t\tgoto out;\n>>>   \t}\n>>>     \t/*\n>>> @@ -359,17 +362,15 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>>>   \tdd2.rchg = xe->xdf2.rchg;\n>>>   \tdd2.rindex = xe->xdf2.rindex;\n>>>   -\tif (xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n>>> -\t\t\t kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0, &xenv) < 0) {\n>>> -\n>>> -\t\txdl_free(kvd);\n>>> -\t\txdl_free_env(xe);\n>>> -\t\treturn -1;\n>>> -\t}\n>>> -\n>>> +\tres = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n>>> +\t\t\t   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,\n>>> +\t\t\t   &xenv);\n>>>   \txdl_free(kvd);\n>>> + out:\n>>> +\tif (res < 0)\n>>> +\t\txdl_free_env(xe);\n>>>   -\treturn 0;\n>>> +\treturn res;\n>>>   }\n>> I wanted to test if this made some diff test pass under\n>> SANITIZE=leak\n>> that didn't before, and my usual quick way to discover that is to run\n>> the tests with something like this for testing:\n>> \t\n>> \tdiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>> \tindex 758410c11ac..7811ce2a1d3 100644\n>> \t--- a/xdiff/xdiffi.c\n>> \t+++ b/xdiff/xdiffi.c\n>> \t@@ -368,7 +368,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>> \t \txdl_free(kvd);\n>> \t  out:\n>> \t \tif (res < 0)\n>> \t-\t\txdl_free_env(xe);\n>> \t+\t\tBUG(\"needed freeing\");\n>> \t\n>> \t \treturn res;\n>> \t }\n>> But doing so has all tests passing, so either this code is\n>> unreachable\n>> or not reachable by any of our tests.\n>\n> Yeah, it is fixing a leak in the error path. The only possible error\n> is an allocation failure and we die rather than returning in that case\n> so the test suite is not going to tell us anything about this patch.\n\nIndeed. I meant that comment as \"here's the first thing I tried, and\nwhen that didn't do anything I went digging further, and...\".\n\n>> More generally I think this patch is taking the wrong approach, why are\n>> we trying to the members of a stack variable in xdl_do_diff(), when that\n>> variable isn't ours, but is created by our caller? Why aren't they\n>> dealing with it?\n>\n> They are not dealing with it because they do not initialize it - it is\n> an \"out\" parameter that is used to return data to the caller. This\n> patch changes the logic to \"whoever initializes it is responsible for\n> freeing it if there is an error\". By doing that we localize the error\n> handling to xdl_do_diff() and can leave the callers unchanged.\n\nYes, I'm saying that we're needlessly piling on complexity by continuing\nwith this pattern in the xdiff/ codebase. I think it's fair to question\nthe direction in general.\n\n>> The main issue the memory handling is such a pain here is because of\n>> that mixed ownership.\n>> The below POC (and seems to pass tests) shows a way to do that,\n>> which I\n>> think in general is *much* simpler. I.e. sure we'll call free()\n>> redundantly some of the time, but that'll be safe as long as we\n>> zero-initialize the relevant struct.\n>> The xdiff code is much harder to read & maintain than it needs to be\n>> because it's trying to micro-optimize these sort of patterns.\n>> E.g. with the diff below we'll now redundantly call the free on a\n>> xe2\n>> when we only used a xe1, but \"who cares?\" is my considered opinion on\n>> that topic :)\n>\n> Then you should reread the cover letter and commit message and code\n> comment in patch 3 that all mention avoiding a double free in that\n> case. If you have any ideas how to make those clearer please let me\n> know.\n\nTo use FREE_AND_NULL() instead of free()? Then double-freeing stops\nbeing an issue, and we don't need to do the accounting.\n\n> If xdl_do_diff(orig, mf2, xpp, &xe2) fails it frees any members of xe2\n> that it allocated but it does not NULL them. If we then call \n> xdl_free_env(&xe2) we will have a double free error.\n\nRight. I'm saying that if you follow the chain from xdl_do_diff() down\nwe call e.g. xdl_do_patience_diff(), which then calls xdl_prepare_env(),\nwhich will on failure call e.g. xdl_free_ctx(&xe->xdf2), i.e. freeing\nspecific things inside the \"xe\" struct that it specifically knows it\nallocated on those codepaths.\n\nIf we instead stick to the rule that he who created the stack variable\nis responsible for freeing it, we generally speaking avoid the need for\nFREE_AND_NULL() over free(), but in either case the control flow becomes\nmuch simpler and less verbose.\n\nI.e. we do away with the incremental freeing because some function 3\nstack frames down knows it populated 1/10 struct fields, instead it all\nbecomes:\n\n 1. Do we own this variable? No, return -1 then. No freeing here.\n 2. The thing that owns the variable calls free_thingy(&x)\n 3. We created \"x\" with \"x = { 0 }\" so calling free() on all its fields is safe\n 4. We do just that.\n\nIt's a lot simpler at the cost of a tiny bit of redundant work, but in\nthese cases we're on the error path anyway, so it won't be a performance\nissue.\n\nThat's how we tend to do it with all of git's internal APIs as far as\nfreeing is concerned, xdiff doesn't do it because it's externally\nimported. But I think it's a better direction, and something that wolud\nwork for both git.git and libgit2.git (and the diff/code size isn't that\ndifferent).\n\n>\n>> By not trying to micro-optimize it like that the memory management and\n>> ownership becomes so much easier to deal with.\n>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>> index 758410c11ac..6ad30a98b62 100644\n>> --- a/xdiff/xdiffi.c\n>> +++ b/xdiff/xdiffi.c\n>> @@ -1054,19 +1054,19 @@ static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n>>   int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>>   \t     xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n>>   \txdchange_t *xscr;\n>> -\txdfenv_t xe;\n>> +\txdfenv_t xe = { 0 };\n>>   \temit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;\n>> +\tint status = 0;\n>>     \tif (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {\n>> -\n>> -\t\treturn -1;\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \tif (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||\n>>   \t    xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||\n>>   \t    xdl_build_script(&xe, &xscr) < 0) {\n>> -\n>> -\t\txdl_free_env(&xe);\n>> -\t\treturn -1;\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \tif (xscr) {\n>>   \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n>> @@ -1078,12 +1078,14 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>>   \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n>>     \t\t\txdl_free_script(xscr);\n>> -\t\t\txdl_free_env(&xe);\n>> -\t\t\treturn -1;\n>> +\t\t\tstatus = -1;\n>> +\t\t\tgoto cleanup;\n>>   \t\t}\n>>   \t\txdl_free_script(xscr);\n>>   \t}\n>> +\n>> +cleanup:\n>>   \txdl_free_env(&xe);\n>>   -\treturn 0;\n>> +\treturn status;\n>>   }\n>> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n>> index fff0b594f9a..4751eab9c12 100644\n>> --- a/xdiff/xmerge.c\n>> +++ b/xdiff/xmerge.c\n>> @@ -685,7 +685,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>>   \t\txmparam_t const *xmp, mmbuffer_t *result)\n>>   {\n>>   \txdchange_t *xscr1, *xscr2;\n>> -\txdfenv_t xe1, xe2;\n>> +\txdfenv_t xe1 = { 0 };\n>> +\txdfenv_t xe2 = { 0 };\n>>   \tint status;\n>>   \txpparam_t const *xpp = &xmp->xpp;\n>>   @@ -693,25 +694,25 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1,\n>> mmfile_t *mf2,\n>>   \tresult->size = 0;\n>>     \tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {\n>> -\t\treturn -1;\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \tif (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {\n>> -\t\txdl_free_env(&xe1);\n>> -\t\treturn -1;\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n>>   \t    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||\n>>   \t    xdl_build_script(&xe1, &xscr1) < 0) {\n>> -\t\txdl_free_env(&xe1);\n>> -\t\treturn -1;\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \tif (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||\n>>   \t    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||\n>>   \t    xdl_build_script(&xe2, &xscr2) < 0) {\n>>   \t\txdl_free_script(xscr1);\n>> -\t\txdl_free_env(&xe1);\n>> -\t\txdl_free_env(&xe2);\n>> -\t\treturn -1;\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>>   \t}\n>>   \tstatus = 0;\n>>   \tif (!xscr1) {\n>> @@ -730,6 +731,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n>>   \txdl_free_script(xscr1);\n>>   \txdl_free_script(xscr2);\n>>   +cleanup:\n>>   \txdl_free_env(&xe1);\n>>   \txdl_free_env(&xe2);\n>>   diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n>> index cfa6e2220ff..63fb2eee975 100644\n>> --- a/xdiff/xutils.c\n>> +++ b/xdiff/xutils.c\n>> @@ -414,7 +414,8 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,\n>>   \t * ranges of lines instead of the whole files.\n>>   \t */\n>>   \tmmfile_t subfile1, subfile2;\n>> -\txdfenv_t env;\n>> +\txdfenv_t env = { 0 };\n>> +\tint status = 0;\n>>     \tsubfile1.ptr = (char *)diff_env->xdf1.recs[line1 - 1]->ptr;\n>>   \tsubfile1.size = diff_env->xdf1.recs[line1 + count1 - 2]->ptr +\n>> @@ -422,13 +423,16 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,\n>>   \tsubfile2.ptr = (char *)diff_env->xdf2.recs[line2 - 1]->ptr;\n>>   \tsubfile2.size = diff_env->xdf2.recs[line2 + count2 - 2]->ptr +\n>>   \t\tdiff_env->xdf2.recs[line2 + count2 - 2]->size - subfile2.ptr;\n>> -\tif (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0)\n>> -\t\treturn -1;\n>> +\tif (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0) {\n>> +\t\tstatus = -1;\n>> +\t\tgoto cleanup;\n>> +\t}\n>>     \tmemcpy(diff_env->xdf1.rchg + line1 - 1, env.xdf1.rchg,\n>> count1);\n>>   \tmemcpy(diff_env->xdf2.rchg + line2 - 1, env.xdf2.rchg, count2);\n>>   +cleanup:\n>>   \txdl_free_env(&env);\n>>   -\treturn 0;\n>> +\treturn status;\n>>   }\n\n"},{"id":"448617","messageId":"xmqq7d9uvji5.fsf@gitster.g","threadId":"57391","inReplyTo":"220216.86h78yuar0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/4] xdiff: fix a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T16:55:30Z","receivedAt":"2022-02-16T16:55:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> They are not dealing with it because they do not initialize it - it is\n>> an \"out\" parameter that is used to return data to the caller. This\n>> patch changes the logic to \"whoever initializes it is responsible for\n>> freeing it if there is an error\". By doing that we localize the error\n>> handling to xdl_do_diff() and can leave the callers unchanged.\n>\n> Yes, I'm saying that we're needlessly piling on complexity by continuing\n> with this pattern in the xdiff/ codebase. I think it's fair to question\n> the direction in general.\n\nIt is perfectly OK to question, but I'd prefer to see local\nconsistency.\n\nThanks.\n\n\n"},{"id":"448680","messageId":"08f1cc5f-672f-f806-49d9-a6d6be3d2a8b@gmail.com","threadId":"57391","inReplyTo":"220216.86h78yuar0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/4] xdiff: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-17T11:05:26Z","receivedAt":"2022-02-17T11:05:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/02/2022 14:38, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>>> More generally I think this patch is taking the wrong approach, why are\n>>> we trying to the members of a stack variable in xdl_do_diff(), when that\n>>> variable isn't ours, but is created by our caller? Why aren't they\n>>> dealing with it?\n>>\n>> They are not dealing with it because they do not initialize it - it is\n>> an \"out\" parameter that is used to return data to the caller. This\n>> patch changes the logic to \"whoever initializes it is responsible for\n>> freeing it if there is an error\". By doing that we localize the error\n>> handling to xdl_do_diff() and can leave the callers unchanged.\n> \n> Yes, I'm saying that we're needlessly piling on complexity by continuing\n> with this pattern in the xdiff/ codebase. I think it's fair to question\n> the direction in general.\n\nI think if there were a lot of these bugs that needed fixing then the \nwholesale changes to the way memory is managed you seem to be suggesting \nwould be worth it. However I think the existing code is mostly correct \nso such a change would be a lot of churn and would absorb a lot of \nreviewer time that would be better spent elsewhere compared to fixing up \nthe code we have.\n\nBest Wishes\n\nPhillip\n"}]}