public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Ramsay Jones <ramsay@ramsayjones•plus.com>
Cc: GIT Mailing-list <git@vger•kernel.org>
Subject: Re: [PATCH 2/2] xdiff/xprepare: fix a memory leak
Date: Fri, 04 Mar 2016 15:50:31 -0800	[thread overview]
Message-ID: <xmqqegbpyf94.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <56DA15FA.1090601@ramsayjones.plus.com> (Ramsay Jones's message of "Fri, 4 Mar 2016 23:10:50 +0000")

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).

I think the latter funkiness came from 9f37c275.

next prev parent reply	other threads:[~2016-03-04 23:50 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-04 23:10 [PATCH 2/2] xdiff/xprepare: fix a memory leak Ramsay Jones
2016-03-04 23:50 ` Junio C Hamano [this message]
2016-03-05  1:19   ` Ramsay Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqegbpyf94.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=ramsay@ramsayjones$(echo .)plus.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox