On Mon, Feb 22, 2016 at 07:52:25PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html