Re: [PATCH] decode_header: fix pointential memory leak if decode_header failed

[Junio C Hamano <gitster@pobox.com>]

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored
> in dec will leak. Simply add strbuf_release and free(dec) will solve
> this problem.

We try to write our proposed log messages so that readers can
understand the idea behind the change without having to look at the
patch.  Even to those who are intimately familiar with this area of
the code base, an exact line number reference rarely add any useful
information.  Something like "In mailinfo.c:decode_header()" would 
help them better than "In mailinfo.c line 539".

> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>     decode_header: fix pointential memory leak if decode_header failed
>     
>     In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in
>     dec will leak. Simply add strbuf_release and free(dec) will solve this
>     problem.

Just FYI, here is a space to describe what would not have to go into
the proposed log message; there is no need to duplicate what you
already said in the log message above.

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v1
> Pull-Request: https://github.com/git/git/pull/1956
>
>  mailinfo.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index 7b001fa5dbd..7a54471a481 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -536,8 +536,11 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
>  			dec = decode_q_segment(&piecebuf, 1);
>  			break;
>  		}
> -		if (convert_to_utf8(mi, dec, charset_q.buf))
> +		if (convert_to_utf8(mi, dec, charset_q.buf)) {
> +			strbuf_release(dec);
> +			free(dec);

OK, this fix is obviously correct.

A nicer fix for longer-term may however be to fix the calling
convention for decode_?_segment() functions, so that they take a
caller-prepared strbuf as a parameter and fill it (and signal an
error by returning -1, a success by returning 0).  There is no way
for them to signal errors they detect (if we do not count the usual
form of doing so by returning NULL, which this caller is not
expecting) with the current calling convention.

We'd still need to release the data in the strbuf "dec" even if we
did so, but the strbuf would be on stack so there is no need to
free().

>  			goto release_return;
> +		}
>  
>  		strbuf_addbuf(&outbuf, dec);
>  		strbuf_release(dec);
>
> base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

Thanks.