threads / patch / 41481

patchxdiff/xmerge: fix memory leak in xdl_merge

Subject: [PATCH] xdiff/xmerge: fix memory leak in xdl_merge

## tl;dr

4 messages between Feb 23, 2016 and Feb 23, 2016. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Patrick Steinhardt· Feb 23, 2016, 08:45 UTC · lore

When building the script for the second file that is to be merged we have already allocated memory for data structures related to the first file. When we encounter an error in building the second script we only free allocated memory related to the second file before erroring out.

Fix this memory leak by also releasing allocated memory related to the first file.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 xdiff/xmerge.c | 2 ++
 1 file changed, 2 insertions(+)

Show changes to xdiff/xmerge.c +2 −0

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index d98f430..d04eb46 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -654,6 +654,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
 	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;
 	}

-- 
2.7.1

Johannes Schindelin· Feb 23, 2016, 10:09 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] xdiff/xmerge: fix memory leak in xdl_merge

Hi Patrick,

On Tue, 23 Feb 2016, Patrick Steinhardt wrote:

Show 5 quoted lines

> When building the script for the second file that is to be merged
> we have already allocated memory for data structures related to
> the first file. When we encounter an error in building the second
> script we only free allocated memory related to the second file
> before erroring out.

ACK.

I wonder, though, whether we need this in addition:

-- snipsnap -- t a/xdiff/xmerge.c b/xdiff/xmerge.c index 625198e..e5c8745 100644

Show changes to xdiff/xmerge.c +5 −2

--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -579,8 +579,11 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t
*mf2,
 	result->ptr = NULL;
 	result->size = 0;
 
-	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0 ||
-			xdl_do_diff(orig, mf2, xpp, &xe2) < 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_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||

Patrick Steinhardt· Feb 23, 2016, 11:51 UTC · re: Johannes Schindelin · lore

Re: [PATCH] xdiff/xmerge: fix memory leak in xdl_merge

On Tue, Feb 23, 2016 at 11:09:50AM +0100, Johannes Schindelin wrote:

Show 34 quoted lines

> Hi Patrick,
> 
> On Tue, 23 Feb 2016, Patrick Steinhardt wrote:
> 
> > When building the script for the second file that is to be merged
> > we have already allocated memory for data structures related to
> > the first file. When we encounter an error in building the second
> > script we only free allocated memory related to the second file
> > before erroring out.
> 
> ACK.
> 
> I wonder, though, whether we need this in addition:
> 
> -- snipsnap --
> t a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 625198e..e5c8745 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -579,8 +579,11 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t
> *mf2,
>  	result->ptr = NULL;
>  	result->size = 0;
>  
> -	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0 ||
> -			xdl_do_diff(orig, mf2, xpp, &xe2) < 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_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||

Oh, yes, this is required. I am somewhat confused by all the pointers being passed around there, it is not immediately obvious which pointers are being allocated and which not when you are not intimate with the code base.

So thanks for pointing this out. Will post v2 soon.

Patrick

Patrick Steinhardt· Feb 23, 2016, 11:59 UTC · re: Patrick Steinhardt · lore

[PATCH v2] xdiff/xmerge: fix memory leak in xdl_merge

Fix this memory leak by also releasing allocated memory related to the first file.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 xdiff/xmerge.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Show changes to xdiff/xmerge.c +7 −2

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index d98f430..f338ad6 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -641,8 +641,11 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
 	result->ptr = NULL;
 	result->size = 0;
 
-	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0 ||
-			xdl_do_diff(orig, mf2, xpp, &xe2) < 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_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||
@@ -654,6 +657,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
 	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;
 	}

-- 
2.7.1