Re: [PATCH 3/3] Correct conditions to free textconv result data

From: Jeff King <peff@peff.net>
Date: Feb 22, 2016, 18:06 UTC
Message-ID: <20160222180645.GB4587@sigill.intra.peff.net>
In-Reply-To: <1456145545-5374-3-git-send-email-pclouds@gmail.com>

On Mon, Feb 22, 2016 at 07:52:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 5 quoted lines

> fill_textconv() have four code paths to return a new buffer. Two of
> them, run_textconv() and notes_cache_get(), return a newly allocated
> buffer. The other two return either a constant string or an existing
> buffer (mmfile_t). We can only free the result buffer if it's allocated
> by fill_textconv(). Correct all call sites.

Right, and those two cases (allocated or not) should follow based on whether you passed in a textconv-enabled driver.

Show 13 quoted lines

> diff --git a/builtin/blame.c b/builtin/blame.c
> index cb6f2fb..b5477ad 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -166,7 +166,7 @@ int textconv_object(const char *path,
>  	df = alloc_filespec(path);
>  	fill_filespec(df, sha1, sha1_valid, mode);
>  	textconv = get_textconv(df);
> -	if (!textconv) {
> +	if (!textconv || !textconv->textconv) {
>  		free_filespec(df);
>  		return 0;
>  	}

This change (and the other similar ones) doesn't make any sense to me. The point of get_textconv() is to return the userdiff driver if and only if it has textconv enabled.

I have a feeling you were confused by the fact that fill_textconv() does:

  if (!driver || !driver->textconv)

to decide whether to allocate the textconv buffer. The latter half of that is really just defensive programming, and I think this would probably be better as:

  if (driver)
	....

  assert(driver->textconv);

to make it more clear that we assume the parameter came from get_textconv().

And if there's a case that triggers that assert(), then I think the bug is in the caller, which should be fixed.

Is there a case I'm missing here where we actually leak memory or try to free non-allocated memory? I didn't see it.

-Peff