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

From: lidongyan <502024330056@smail.nju.edu.cn>
Date: May 9, 2025, 01:51 UTC
Message-ID: <19EA1FF8-836C-4444-A027-A65CF28E2CFE@smail.nju.edu.cn>
In-Reply-To: <xmqqh61u4ul4.fsf@gitster.g>

Thank you for your suggestion. I will update the log message in next patch.

The only reason decode_?_header might fail is that xmalloc could 
return NULL, whereas other functions will cause the program to terminate
 on failure. If I pass a pointer to a local variable into these functions,
decode_?_header will always return 0. Should I change decode_?_header
signature in the next patch?

Show 72 quoted lines

> 2025年5月9日 06:16，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.