"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.
