{"thread":{"id":"41481","subject":"[PATCH] xdiff/xmerge: fix memory leak in xdl_merge","startedAt":"2016-02-23T08:45:51Z","lastAt":"2016-02-23T11:59:17Z","messageCount":4,"participants":["Patrick Steinhardt","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"278985","messageId":"1456217151-16473-1-git-send-email-ps@pks.im","threadId":"41481","inReplyTo":null,"subject":"[PATCH] xdiff/xmerge: fix memory leak in xdl_merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-02-23T08:45:51Z","receivedAt":"2016-02-23T08:45:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When building the script for the second file that is to be merged\nwe have already allocated memory for data structures related to\nthe first file. When we encounter an error in building the second\nscript we only free allocated memory related to the second file\nbefore erroring out.\n\nFix this memory leak by also releasing allocated memory related\nto the first file.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n xdiff/xmerge.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex d98f430..d04eb46 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -654,6 +654,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\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-- \n2.7.1\n"},{"id":"278990","messageId":"alpine.DEB.2.20.1602231108260.3152@virtualbox","threadId":"41481","inReplyTo":"1456217151-16473-1-git-send-email-ps@pks.im","subject":"Re: [PATCH] xdiff/xmerge: fix memory leak in xdl_merge","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-23T10:09:50Z","receivedAt":"2016-02-23T10:09:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Tue, 23 Feb 2016, Patrick Steinhardt wrote:\n\n> When building the script for the second file that is to be merged\n> we have already allocated memory for data structures related to\n> the first file. When we encounter an error in building the second\n> script we only free allocated memory related to the second file\n> before erroring out.\n\nACK.\n\nI wonder, though, whether we need this in addition:\n\n-- snipsnap --\nt a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 625198e..e5c8745 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -579,8 +579,11 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t\n*mf2,\n \tresult->ptr = NULL;\n \tresult->size = 0;\n \n-\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0 ||\n-\t\t\txdl_do_diff(orig, mf2, xpp, &xe2) < 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 \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n"},{"id":"278994","messageId":"20160223115135.GA977@pks-pc.localdomain","threadId":"41481","inReplyTo":"alpine.DEB.2.20.1602231108260.3152@virtualbox","subject":"Re: [PATCH] xdiff/xmerge: fix memory leak in xdl_merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-02-23T11:51:35Z","receivedAt":"2016-02-23T11:51:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 23, 2016 at 11:09:50AM +0100, Johannes Schindelin wrote:\n> Hi Patrick,\n> \n> On Tue, 23 Feb 2016, Patrick Steinhardt wrote:\n> \n> > When building the script for the second file that is to be merged\n> > we have already allocated memory for data structures related to\n> > the first file. When we encounter an error in building the second\n> > script we only free allocated memory related to the second file\n> > before erroring out.\n> \n> ACK.\n> \n> I wonder, though, whether we need this in addition:\n> \n> -- snipsnap --\n> t a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index 625198e..e5c8745 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -579,8 +579,11 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t\n> *mf2,\n>  \tresult->ptr = NULL;\n>  \tresult->size = 0;\n>  \n> -\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0 ||\n> -\t\t\txdl_do_diff(orig, mf2, xpp, &xe2) < 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>  \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n\nOh, yes, this is required. I am somewhat confused by all the\npointers being passed around there, it is not immediately obvious\nwhich pointers are being allocated and which not when you are not\nintimate with the code base.\n\nSo thanks for pointing this out. Will post v2 soon.\n\nPatrick\n"},{"id":"278995","messageId":"1456228757-19333-1-git-send-email-ps@pks.im","threadId":"41481","inReplyTo":"1456217151-16473-1-git-send-email-ps@pks.im","subject":"[PATCH v2] xdiff/xmerge: fix memory leak in xdl_merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-02-23T11:59:17Z","receivedAt":"2016-02-23T11:59:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When building the script for the second file that is to be merged\nwe have already allocated memory for data structures related to\nthe first file. When we encounter an error in building the second\nscript we only free allocated memory related to the second file\nbefore erroring out.\n\nFix this memory leak by also releasing allocated memory related\nto the first file.\n\nHelped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n xdiff/xmerge.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex d98f430..f338ad6 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -641,8 +641,11 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \tresult->ptr = NULL;\n \tresult->size = 0;\n \n-\tif (xdl_do_diff(orig, mf1, xpp, &xe1) < 0 ||\n-\t\t\txdl_do_diff(orig, mf2, xpp, &xe2) < 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 \tif (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||\n@@ -654,6 +657,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\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-- \n2.7.1\n"}]}