{"thread":{"id":"41615","subject":"[PATCH 2/2] xdiff/xprepare: fix a memory leak","startedAt":"2016-03-04T23:10:50Z","lastAt":"2016-03-05T01:19:18Z","messageCount":3,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"280217","messageId":"56DA15FA.1090601@ramsayjones.plus.com","threadId":"41615","inReplyTo":null,"subject":"[PATCH 2/2] xdiff/xprepare: fix a memory leak","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-03-04T23:10:50Z","receivedAt":"2016-03-04T23:10:50Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe xdl_prepare_env() function may initialise an xdlclassifier_t\ndata structure via xdl_init_classifier(), which allocates memory\nto several fields, for example 'rchash', 'rcrecs' and 'ncha'.\nIf this function later exits due to the failure of xdl_optimize_ctxs(),\nthen this xdlclassifier_t structure, and the memory allocated to it,\nis not cleaned up.\n\nIn order to fix the memory leak, insert a call to xdl_free_classifier()\nbefore returning.\n\nSigned-off-by: Ramsay Jones \n---\n xdiff/xprepare.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\nindex 5ffcf99..13b55ab 100644\n--- a/xdiff/xprepare.c\n+++ b/xdiff/xprepare.c\n@@ -301,6 +301,7 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \n \t\txdl_free_ctx(&xe->xdf2);\n \t\txdl_free_ctx(&xe->xdf1);\n+\t\txdl_free_classifier(&cf);\n \t\treturn -1;\n \t}\n \n-- \n2.7.0\n"},{"id":"280219","messageId":"xmqqegbpyf94.fsf@gitster.mtv.corp.google.com","threadId":"41615","inReplyTo":"56DA15FA.1090601@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-04T23:50:31Z","receivedAt":"2016-03-04T23:50:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> The xdl_prepare_env() function may initialise an xdlclassifier_t\n> data structure via xdl_init_classifier(), which allocates memory\n> to several fields, for example 'rchash', 'rcrecs' and 'ncha'.\n> If this function later exits due to the failure of xdl_optimize_ctxs(),\n> then this xdlclassifier_t structure, and the memory allocated to it,\n> is not cleaned up.\n>\n> In order to fix the memory leak, insert a call to xdl_free_classifier()\n> before returning.\n>\n> Signed-off-by: Ramsay Jones \n> ---\n> xdiff/xprepare.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\n> index 5ffcf99..13b55ab 100644\n> --- a/xdiff/xprepare.c\n> +++ b/xdiff/xprepare.c\n> @@ -301,6 +301,7 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \n> \t\txdl_free_ctx(&xe->xdf2);\n> \t\txdl_free_ctx(&xe->xdf1);\n> +\t\txdl_free_classifier(&cf);\n> \t\treturn -1;\n> \t}\n\nThis looks obviously correct from the pattern of prepare's and\nfree's in the code that this part follows. This potential leak has\nbeen that way since 3443546f (Use a *real* built-in diff generator,\n2006-03-24), i.e. the very beginning.\n\nI find it somewhat strange that the call to xdl_free_classifier() at\nthe end of this function is made conditional to XDF_HISTOGRAM_DIFF,\nthough. I can half-buy the argument \"that is because we do not call\ninit-classifier for XDF_HISTOGRAM_DIFF\", but in the error path we\ncall free-classifier unconditionally, so the code clearly knows that\nit is safe to call free-classifier on a classifier that is cleared\nwith the initial memset(&cf, 0, sizeof cf).\n\nI think the latter funkiness came from 9f37c275.\n"},{"id":"280225","messageId":"56DA3416.10707@ramsayjones.plus.com","threadId":"41615","inReplyTo":"xmqqegbpyf94.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-03-05T01:19:18Z","receivedAt":"2016-03-05T01:19:18Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/03/16 23:50, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> The xdl_prepare_env() function may initialise an xdlclassifier_t\n>> data structure via xdl_init_classifier(), which allocates memory\n>> to several fields, for example 'rchash', 'rcrecs' and 'ncha'.\n>> If this function later exits due to the failure of xdl_optimize_ctxs(),\n>> then this xdlclassifier_t structure, and the memory allocated to it,\n>> is not cleaned up.\n>>\n>> In order to fix the memory leak, insert a call to xdl_free_classifier()\n>> before returning.\n>>\n>> Signed-off-by: Ramsay Jones \n>> ---\n>> xdiff/xprepare.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\n>> index 5ffcf99..13b55ab 100644\n>> --- a/xdiff/xprepare.c\n>> +++ b/xdiff/xprepare.c\n>> @@ -301,6 +301,7 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n>> \n>> \t\txdl_free_ctx(&xe->xdf2);\n>> \t\txdl_free_ctx(&xe->xdf1);\n>> +\t\txdl_free_classifier(&cf);\n>> \t\treturn -1;\n>> \t}\n> \n> This looks obviously correct from the pattern of prepare's and\n> free's in the code that this part follows. This potential leak has\n> been that way since 3443546f (Use a *real* built-in diff generator,\n> 2006-03-24), i.e. the very beginning.\n> \n> I find it somewhat strange that the call to xdl_free_classifier() at\n> the end of this function is made conditional to XDF_HISTOGRAM_DIFF,\n> though. I can half-buy the argument \"that is because we do not call\n> init-classifier for XDF_HISTOGRAM_DIFF\", but in the error path we\n> call free-classifier unconditionally, so the code clearly knows that\n> it is safe to call free-classifier on a classifier that is cleared\n> with the initial memset(&cf, 0, sizeof cf).\n\nIndeed, this is actually why I noticed that XDF_DIFF_ALG() wasn't used.\nRather than doing patch #1, I did consider making this call unconditional.\nI can't remember why I didn't. (Hmm, perhaps I just chickened out! ;-))\n\nATB,\nRamsay Jones\n"}]}