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? > 2025年5月9日 06:16，Junio C Hamano <gitster@xxxxxxxxx> 写道： > > "Lidong Yan via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxxx> >> --- >> 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.
