threads / patch / 41615

patch, 2 partsxdiff/xprepare: fix a memory leak

Subject: [PATCH 2/2] xdiff/xprepare: fix a memory leak

## tl;dr

3 messages between Mar 4, 2016 and Mar 5, 2016. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Ramsay Jones· Mar 4, 2016, 23:10 UTC · lore

The xdl_prepare_env() function may initialise an xdlclassifier_t data structure via xdl_init_classifier(), which allocates memory to several fields, for example 'rchash', 'rcrecs' and 'ncha'. If this function later exits due to the failure of xdl_optimize_ctxs(), then this xdlclassifier_t structure, and the memory allocated to it, is not cleaned up.

In order to fix the memory leak, insert a call to xdl_free_classifier() before returning.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 xdiff/xprepare.c | 1 +
 1 file changed, 1 insertion(+)

Show changes to xdiff/xprepare.c +1 −0

diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index 5ffcf99..13b55ab 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -301,6 +301,7 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 
 		xdl_free_ctx(&xe->xdf2);
 		xdl_free_ctx(&xe->xdf1);
+		xdl_free_classifier(&cf);
 		return -1;
 	}

-- 
2.7.0

Junio C Hamano· Mar 4, 2016, 23:50 UTC · re: Ramsay Jones · lore

Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

Show 26 quoted lines

> The xdl_prepare_env() function may initialise an xdlclassifier_t
> data structure via xdl_init_classifier(), which allocates memory
> to several fields, for example 'rchash', 'rcrecs' and 'ncha'.
> If this function later exits due to the failure of xdl_optimize_ctxs(),
> then this xdlclassifier_t structure, and the memory allocated to it,
> is not cleaned up.
>
> In order to fix the memory leak, insert a call to xdl_free_classifier()
> before returning.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>  xdiff/xprepare.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index 5ffcf99..13b55ab 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -301,6 +301,7 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>  
>  		xdl_free_ctx(&xe->xdf2);
>  		xdl_free_ctx(&xe->xdf1);
> +		xdl_free_classifier(&cf);
>  		return -1;
>  	}

This looks obviously correct from the pattern of prepare's and free's in the code that this part follows. This potential leak has been that way since 3443546f (Use a *real* built-in diff generator, 2006-03-24), i.e. the very beginning.

I find it somewhat strange that the call to xdl_free_classifier() at the end of this function is made conditional to XDF_HISTOGRAM_DIFF, though. I can half-buy the argument "that is because we do not call init-classifier for XDF_HISTOGRAM_DIFF", but in the error path we call free-classifier unconditionally, so the code clearly knows that it is safe to call free-classifier on a classifier that is cleared with the initial memset(&cf, 0, sizeof cf).

I think the latter funkiness came from 9f37c275.

Ramsay Jones· Mar 5, 2016, 01:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak

On 04/03/16 23:50, Junio C Hamano wrote:

Show 41 quoted lines

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> The xdl_prepare_env() function may initialise an xdlclassifier_t
>> data structure via xdl_init_classifier(), which allocates memory
>> to several fields, for example 'rchash', 'rcrecs' and 'ncha'.
>> If this function later exits due to the failure of xdl_optimize_ctxs(),
>> then this xdlclassifier_t structure, and the memory allocated to it,
>> is not cleaned up.
>>
>> In order to fix the memory leak, insert a call to xdl_free_classifier()
>> before returning.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>>  xdiff/xprepare.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
>> index 5ffcf99..13b55ab 100644
>> --- a/xdiff/xprepare.c
>> +++ b/xdiff/xprepare.c
>> @@ -301,6 +301,7 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>  
>>  		xdl_free_ctx(&xe->xdf2);
>>  		xdl_free_ctx(&xe->xdf1);
>> +		xdl_free_classifier(&cf);
>>  		return -1;
>>  	}
> 
> This looks obviously correct from the pattern of prepare's and
> free's in the code that this part follows.  This potential leak has
> been that way since 3443546f (Use a *real* built-in diff generator,
> 2006-03-24), i.e. the very beginning.
> 
> I find it somewhat strange that the call to xdl_free_classifier() at
> the end of this function is made conditional to XDF_HISTOGRAM_DIFF,
> though.  I can half-buy the argument "that is because we do not call
> init-classifier for XDF_HISTOGRAM_DIFF", but in the error path we
> call free-classifier unconditionally, so the code clearly knows that
> it is safe to call free-classifier on a classifier that is cleared
> with the initial memset(&cf, 0, sizeof cf).

Indeed, this is actually why I noticed that XDF_DIFF_ALG() wasn't used. Rather than doing patch #1, I did consider making this call unconditional. I can't remember why I didn't. (Hmm, perhaps I just chickened out! ;-))

ATB, Ramsay Jones