# [PATCH 2/2] xdiff/xprepare: fix a memory leak 3 messages from 2016-03-04 to 2016-03-05. Participants: Ramsay Jones, Junio C Hamano. Thread: https://gitlist.dev/t/41615 ## Ramsay Jones, 2016-03-04 23:10 Subject: [PATCH 2/2] xdiff/xprepare: fix a memory leak Message-ID: <56DA15FA.1090601@ramsayjones.plus.com> URL: https://gitlist.dev/e/56DA15FA.1090601%40ramsayjones.plus.com ``` 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 --- 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; } -- 2.7.0 ``` ## Junio C Hamano, 2016-03-04 23:50 Subject: Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak Message-ID: URL: https://gitlist.dev/e/xmqqegbpyf94.fsf%40gitster.mtv.corp.google.com In-Reply-To: <56DA15FA.1090601@ramsayjones.plus.com> ``` Ramsay Jones 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 > --- > 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, 2016-03-05 01:19 Subject: Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak Message-ID: <56DA3416.10707@ramsayjones.plus.com> URL: https://gitlist.dev/e/56DA3416.10707%40ramsayjones.plus.com In-Reply-To: ``` On 04/03/16 23:50, Junio C Hamano wrote: > Ramsay Jones 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 >> --- >> 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 ```