{"thread":{"id":"63419","subject":"[PATCH] decode_header: fix pointential memory leak if decode_header failed","startedAt":"2025-05-08T13:38:44Z","lastAt":"2025-05-13T13:42:57Z","messageCount":10,"participants":["Lidong Yan via GitGitGadget","Junio C Hamano","lidongyan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"517550","messageId":"pull.1956.git.git.1746711521614.gitgitgadget@gmail.com","threadId":"63419","inReplyTo":null,"subject":"[PATCH] decode_header: fix pointential memory leak if decode_header failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-08T13:38:41Z","receivedAt":"2025-05-08T13:38:44Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored\nin dec will leak. Simply add strbuf_release and free(dec) will solve\nthis problem.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    decode_header: fix pointential memory leak if decode_header failed\n    \n    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n    dec will leak. Simply add strbuf_release and free(dec) will solve this\n    problem.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v1\nPull-Request: https://github.com/git/git/pull/1956\n\n mailinfo.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 7b001fa5dbd..7a54471a481 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -536,8 +536,11 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n \t\t\tdec = decode_q_segment(&piecebuf, 1);\n \t\t\tbreak;\n \t\t}\n-\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n+\t\tif (convert_to_utf8(mi, dec, charset_q.buf)) {\n+\t\t\tstrbuf_release(dec);\n+\t\t\tfree(dec);\n \t\t\tgoto release_return;\n+\t\t}\n \n \t\tstrbuf_addbuf(&outbuf, dec);\n \t\tstrbuf_release(dec);\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"517605","messageId":"xmqqh61u4ul4.fsf@gitster.g","threadId":"63419","inReplyTo":"pull.1956.git.git.1746711521614.gitgitgadget@gmail.com","subject":"Re: [PATCH] decode_header: fix pointential memory leak if decode_header failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-08T22:16:07Z","receivedAt":"2025-05-08T22:16:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored\n> in dec will leak. Simply add strbuf_release and free(dec) will solve\n> this problem.\n\nWe try to write our proposed log messages so that readers can\nunderstand the idea behind the change without having to look at the\npatch.  Even to those who are intimately familiar with this area of\nthe code base, an exact line number reference rarely add any useful\ninformation.  Something like \"In mailinfo.c:decode_header()\" would \nhelp them better than \"In mailinfo.c line 539\".\n\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n>     decode_header: fix pointential memory leak if decode_header failed\n>     \n>     In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n>     dec will leak. Simply add strbuf_release and free(dec) will solve this\n>     problem.\n\nJust FYI, here is a space to describe what would not have to go into\nthe proposed log message; there is no need to duplicate what you\nalready said in the log message above.\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v1\n> Pull-Request: https://github.com/git/git/pull/1956\n>\n>  mailinfo.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 7b001fa5dbd..7a54471a481 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -536,8 +536,11 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n>  \t\t\tdec = decode_q_segment(&piecebuf, 1);\n>  \t\t\tbreak;\n>  \t\t}\n> -\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n> +\t\tif (convert_to_utf8(mi, dec, charset_q.buf)) {\n> +\t\t\tstrbuf_release(dec);\n> +\t\t\tfree(dec);\n\nOK, this fix is obviously correct.\n\nA nicer fix for longer-term may however be to fix the calling\nconvention for decode_?_segment() functions, so that they take a\ncaller-prepared strbuf as a parameter and fill it (and signal an\nerror by returning -1, a success by returning 0).  There is no way\nfor them to signal errors they detect (if we do not count the usual\nform of doing so by returning NULL, which this caller is not\nexpecting) with the current calling convention.\n\nWe'd still need to release the data in the strbuf \"dec\" even if we\ndid so, but the strbuf would be on stack so there is no need to\nfree().\n\n>  \t\t\tgoto release_return;\n> +\t\t}\n>  \n>  \t\tstrbuf_addbuf(&outbuf, dec);\n>  \t\tstrbuf_release(dec);\n>\n> base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n\nThanks.\n"},{"id":"517617","messageId":"19EA1FF8-836C-4444-A027-A65CF28E2CFE@smail.nju.edu.cn","threadId":"63419","inReplyTo":"xmqqh61u4ul4.fsf@gitster.g","subject":"Re: [PATCH] decode_header: fix pointential memory leak if decode_header failed","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-09T01:51:58Z","receivedAt":"2025-05-09T01:53:12Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Thank you for your suggestion. I will update the log message in next\npatch.\n\nThe only reason decode_?_header might fail is that xmalloc could \nreturn NULL, whereas other functions will cause the program to terminate\n on failure. If I pass a pointer to a local variable into these functions,\ndecode_?_header will always return 0. Should I change decode_?_header\nsignature in the next patch?\n\n> 2025年5月9日 06:16，Junio C Hamano <gitster@pobox.com> 写道：\n> \n> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored\n>> in dec will leak. Simply add strbuf_release and free(dec) will solve\n>> this problem.\n> \n> We try to write our proposed log messages so that readers can\n> understand the idea behind the change without having to look at the\n> patch.  Even to those who are intimately familiar with this area of\n> the code base, an exact line number reference rarely add any useful\n> information.  Something like \"In mailinfo.c:decode_header()\" would \n> help them better than \"In mailinfo.c line 539\".\n> \n>> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> ---\n>>    decode_header: fix pointential memory leak if decode_header failed\n>> \n>>    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n>>    dec will leak. Simply add strbuf_release and free(dec) will solve this\n>>    problem.\n> \n> Just FYI, here is a space to describe what would not have to go into\n> the proposed log message; there is no need to duplicate what you\n> already said in the log message above.\n> \n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v1\n>> Pull-Request: https://github.com/git/git/pull/1956\n>> \n>> mailinfo.c | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/mailinfo.c b/mailinfo.c\n>> index 7b001fa5dbd..7a54471a481 100644\n>> --- a/mailinfo.c\n>> +++ b/mailinfo.c\n>> @@ -536,8 +536,11 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n>> dec = decode_q_segment(&piecebuf, 1);\n>> break;\n>> }\n>> - if (convert_to_utf8(mi, dec, charset_q.buf))\n>> + if (convert_to_utf8(mi, dec, charset_q.buf)) {\n>> + strbuf_release(dec);\n>> + free(dec);\n> \n> OK, this fix is obviously correct.\n> \n> A nicer fix for longer-term may however be to fix the calling\n> convention for decode_?_segment() functions, so that they take a\n> caller-prepared strbuf as a parameter and fill it (and signal an\n> error by returning -1, a success by returning 0).  There is no way\n> for them to signal errors they detect (if we do not count the usual\n> form of doing so by returning NULL, which this caller is not\n> expecting) with the current calling convention.\n> \n> We'd still need to release the data in the strbuf \"dec\" even if we\n> did so, but the strbuf would be on stack so there is no need to\n> free().\n> \n>> goto release_return;\n>> + }\n>> \n>> strbuf_addbuf(&outbuf, dec);\n>> strbuf_release(dec);\n>> \n>> base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n> \n> Thanks.\n\n\n"},{"id":"517786","messageId":"pull.1956.v2.git.git.1746980097510.gitgitgadget@gmail.com","threadId":"63419","inReplyTo":"pull.1956.git.git.1746711521614.gitgitgadget@gmail.com","subject":"[PATCH v2] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-11T16:14:57Z","receivedAt":"2025-05-11T16:15:01Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored\nin dec will leak. Simply add strbuf_release and free(dec) will solve\nthis problem.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    decode_header: fix pointential memory leak if decode_header failed\n    \n    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n    dec will leak. Simply add strbuf_release and free(dec) will solve this\n    problem.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v2\nPull-Request: https://github.com/git/git/pull/1956\n\nRange-diff vs v1:\n\n 1:  81fdfb94315 ! 1:  90dc9b0d49b decode_header: fix pointential memory leak if decode_header failed\n     @@ Metadata\n      Author: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n       ## Commit message ##\n     -    decode_header: fix pointential memory leak if decode_header failed\n     +    mailinfo: fix pointential memory leak if `decode_header` failed\n      \n     -    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored\n     +    In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored\n          in dec will leak. Simply add strbuf_release and free(dec) will solve\n          this problem.\n      \n          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n       ## mailinfo.c ##\n     +@@ mailinfo.c: static int is_format_patch_separator(const char *line, int len)\n     + \treturn !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));\n     + }\n     + \n     +-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n     ++static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n     ++\t\t\t    int rfc2047)\n     + {\n     + \tconst char *in = q_seg->buf;\n     + \tint c;\n     +-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n     + \tstrbuf_init(out, q_seg->len);\n     + \n     + \twhile ((c = *in++) != 0) {\n     +@@ mailinfo.c: static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n     + \t\t\tc = 0x20;\n     + \t\tstrbuf_addch(out, c);\n     + \t}\n     +-\treturn out;\n     ++\treturn 0;\n     + }\n     + \n     +-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n     ++static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n     + {\n     + \t/* Decode in..ep, possibly in-place to ot */\n     + \tint c, pos = 0, acc = 0;\n     + \tconst char *in = b_seg->buf;\n     +-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n     + \tstrbuf_init(out, b_seg->len);\n     + \n     + \twhile ((c = *in++) != 0) {\n     +@@ mailinfo.c: static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n     + \t\t\tbreak;\n     + \t\t}\n     + \t}\n     +-\treturn out;\n     ++\treturn 0;\n     + }\n     + \n     + static int convert_to_utf8(struct mailinfo *mi,\n     +@@ mailinfo.c: static int convert_to_utf8(struct mailinfo *mi,\n     + static void decode_header(struct mailinfo *mi, struct strbuf *it)\n     + {\n     + \tchar *in, *ep, *cp;\n     +-\tstruct strbuf outbuf = STRBUF_INIT, *dec;\n     ++\tstruct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;\n     + \tstruct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;\n     + \tint found_error = 1; /* pessimism */\n     + \n      @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)\n     - \t\t\tdec = decode_q_segment(&piecebuf, 1);\n     + \t\tdefault:\n     + \t\t\tgoto release_return;\n     + \t\tcase 'b':\n     +-\t\t\tdec = decode_b_segment(&piecebuf);\n     ++\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf))) {\n     ++\t\t\t\tgoto release_return;\n     ++\t\t\t}\n     + \t\t\tbreak;\n     + \t\tcase 'q':\n     +-\t\t\tdec = decode_q_segment(&piecebuf, 1);\n     ++\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {\n     ++\t\t\t\tgoto release_return;\n     ++\t\t\t}\n       \t\t\tbreak;\n       \t\t}\n      -\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n     -+\t\tif (convert_to_utf8(mi, dec, charset_q.buf)) {\n     -+\t\t\tstrbuf_release(dec);\n     -+\t\t\tfree(dec);\n     ++\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n     ++\t\t\tstrbuf_release(&dec);\n       \t\t\tgoto release_return;\n      +\t\t}\n       \n     - \t\tstrbuf_addbuf(&outbuf, dec);\n     - \t\tstrbuf_release(dec);\n     +-\t\tstrbuf_addbuf(&outbuf, dec);\n     +-\t\tstrbuf_release(dec);\n     +-\t\tfree(dec);\n     ++\t\tstrbuf_addbuf(&outbuf, &dec);\n     ++\t\tstrbuf_release(&dec);\n     + \t\tin = ep + 2;\n     + \t}\n     + \tstrbuf_addstr(&outbuf, in);\n     +@@ mailinfo.c: static int is_inbody_header(const struct mailinfo *mi,\n     + \n     + static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n     + {\n     +-\tstruct strbuf *ret;\n     ++\tstruct strbuf ret;\n     ++\tint found_error = 0;\n     + \n     + \tswitch (mi->transfer_encoding) {\n     + \tcase TE_QP:\n     +-\t\tret = decode_q_segment(line, 0);\n     ++\t\tfound_error = decode_q_segment(&ret, line, 0);\n     + \t\tbreak;\n     + \tcase TE_BASE64:\n     +-\t\tret = decode_b_segment(line);\n     ++\t\tfound_error = decode_b_segment(&ret, line);\n     + \t\tbreak;\n     + \tcase TE_DONTCARE:\n     + \tdefault:\n     + \t\treturn;\n     + \t}\n     + \tstrbuf_reset(line);\n     +-\tstrbuf_addbuf(line, ret);\n     +-\tstrbuf_release(ret);\n     +-\tfree(ret);\n     ++\tstrbuf_addbuf(line, &ret);\n     ++\tif (!found_error)\n     ++\t\tstrbuf_release(&ret);\n     + }\n     + \n     + static inline int patchbreak(const struct strbuf *line)\n\n\n mailinfo.c | 43 ++++++++++++++++++++++++-------------------\n 1 file changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 7b001fa5dbd..11ec3914ce4 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -381,11 +381,11 @@ static int is_format_patch_separator(const char *line, int len)\n \treturn !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));\n }\n \n-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n+static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n+\t\t\t    int rfc2047)\n {\n \tconst char *in = q_seg->buf;\n \tint c;\n-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n \tstrbuf_init(out, q_seg->len);\n \n \twhile ((c = *in++) != 0) {\n@@ -405,15 +405,14 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n \t\t\tc = 0x20;\n \t\tstrbuf_addch(out, c);\n \t}\n-\treturn out;\n+\treturn 0;\n }\n \n-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n+static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n {\n \t/* Decode in..ep, possibly in-place to ot */\n \tint c, pos = 0, acc = 0;\n \tconst char *in = b_seg->buf;\n-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n \tstrbuf_init(out, b_seg->len);\n \n \twhile ((c = *in++) != 0) {\n@@ -447,7 +446,7 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n \t\t\tbreak;\n \t\t}\n \t}\n-\treturn out;\n+\treturn 0;\n }\n \n static int convert_to_utf8(struct mailinfo *mi,\n@@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n static void decode_header(struct mailinfo *mi, struct strbuf *it)\n {\n \tchar *in, *ep, *cp;\n-\tstruct strbuf outbuf = STRBUF_INIT, *dec;\n+\tstruct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;\n \tstruct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;\n \tint found_error = 1; /* pessimism */\n \n@@ -530,18 +529,23 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n \t\tdefault:\n \t\t\tgoto release_return;\n \t\tcase 'b':\n-\t\t\tdec = decode_b_segment(&piecebuf);\n+\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf))) {\n+\t\t\t\tgoto release_return;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 'q':\n-\t\t\tdec = decode_q_segment(&piecebuf, 1);\n+\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {\n+\t\t\t\tgoto release_return;\n+\t\t\t}\n \t\t\tbreak;\n \t\t}\n-\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n+\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n+\t\t\tstrbuf_release(&dec);\n \t\t\tgoto release_return;\n+\t\t}\n \n-\t\tstrbuf_addbuf(&outbuf, dec);\n-\t\tstrbuf_release(dec);\n-\t\tfree(dec);\n+\t\tstrbuf_addbuf(&outbuf, &dec);\n+\t\tstrbuf_release(&dec);\n \t\tin = ep + 2;\n \t}\n \tstrbuf_addstr(&outbuf, in);\n@@ -634,23 +638,24 @@ static int is_inbody_header(const struct mailinfo *mi,\n \n static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n {\n-\tstruct strbuf *ret;\n+\tstruct strbuf ret;\n+\tint found_error = 0;\n \n \tswitch (mi->transfer_encoding) {\n \tcase TE_QP:\n-\t\tret = decode_q_segment(line, 0);\n+\t\tfound_error = decode_q_segment(&ret, line, 0);\n \t\tbreak;\n \tcase TE_BASE64:\n-\t\tret = decode_b_segment(line);\n+\t\tfound_error = decode_b_segment(&ret, line);\n \t\tbreak;\n \tcase TE_DONTCARE:\n \tdefault:\n \t\treturn;\n \t}\n \tstrbuf_reset(line);\n-\tstrbuf_addbuf(line, ret);\n-\tstrbuf_release(ret);\n-\tfree(ret);\n+\tstrbuf_addbuf(line, &ret);\n+\tif (!found_error)\n+\t\tstrbuf_release(&ret);\n }\n \n static inline int patchbreak(const struct strbuf *line)\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"517849","messageId":"xmqqcycdx6iv.fsf@gitster.g","threadId":"63419","inReplyTo":"pull.1956.v2.git.git.1746980097510.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-12T14:12:08Z","receivedAt":"2025-05-12T14:12:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored\n> in dec will leak. Simply add strbuf_release and free(dec) will solve\n> this problem.\n>\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n>     decode_header: fix pointential memory leak if decode_header failed\n>     \n>     In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n>     dec will leak. Simply add strbuf_release and free(dec) will solve this\n>     problem.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v2\n> Pull-Request: https://github.com/git/git/pull/1956\n>\n> Range-diff vs v1:\n>\n>  1:  81fdfb94315 ! 1:  90dc9b0d49b decode_header: fix pointential memory leak if decode_header failed\n>      @@ Metadata\n>       Author: Lidong Yan <502024330056@smail.nju.edu.cn>\n>       \n>        ## Commit message ##\n>      -    decode_header: fix pointential memory leak if decode_header failed\n>      +    mailinfo: fix pointential memory leak if `decode_header` failed\n>       \n>      -    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored\n>      +    In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored\n>           in dec will leak. Simply add strbuf_release and free(dec) will solve\n>           this problem.\n\nMuch better.\n\n> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n> +\t\t\t    int rfc2047)\n>  {\n>  \tconst char *in = q_seg->buf;\n>  \tint c;\n> -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n>  \tstrbuf_init(out, q_seg->len);\n\nDon't let the caller pass in an uninitialized thing and force the\ncallee to initialize it.  Drop this strbuf_init(), and make the\ncaller always do:\n\n\tstruct strbuf dec = STRBUF_INIT;\n\n\t...\n\t\tdecode_q_segment(&dec, ...);\n\ninstead.  That makes the division of labor easier to see (e.g., what\nif the caller had a code path that never calls decode_x_segment()\nbefore it has to return?  it might want to add something to dec\nitself so that it can base its behaviour always on what is in dec,\nor at the end it may just be able to strbuf_release(&dec) without\nhaving to remember if it called decode_x_segment().  Which means it\nis more convenient for it to always assume that dec is initialized\nwhether it called decode_x_segment() or not).\n\n> -static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n> +static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n>  {\n>  \t/* Decode in..ep, possibly in-place to ot */\n>  \tint c, pos = 0, acc = 0;\n>  \tconst char *in = b_seg->buf;\n> -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n>  \tstrbuf_init(out, b_seg->len);\n\nDitto.\n\n> @@ -530,18 +529,23 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n>  \t\tdefault:\n>  \t\t\tgoto release_return;\n>  \t\tcase 'b':\n> -\t\t\tdec = decode_b_segment(&piecebuf);\n> +\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf))) {\n> +\t\t\t\tgoto release_return;\n> +\t\t\t}\n\nDon't enclose a single statement block inside {braces}.\n\n>  \t\t\tbreak;\n>  \t\tcase 'q':\n> -\t\t\tdec = decode_q_segment(&piecebuf, 1);\n> +\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {\n> +\t\t\t\tgoto release_return;\n> +\t\t\t}\n\nDitto.\n\n>  \t\t\tbreak;\n\nJust a mental note (i.e., not anything wrong in the posted patch).\nEven though the caller is prepared to see decode_x_segment() to\nnotice and report an error, the implementation just does not bother,\nand mostly skips a garbage in the input.  Outside the topic of this\nseries, we may want to consider allowing the user to say \"be strict\nand barf when encoded contents are broken\".\n\n>  \t\t}\n> -\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n> +\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n> +\t\t\tstrbuf_release(&dec);\n>  \t\t\tgoto release_return;\n> +\t\t}\n\nThis, together with ...\n\n> -\t\tstrbuf_addbuf(&outbuf, dec);\n> -\t\tstrbuf_release(dec);\n> -\t\tfree(dec);\n> +\t\tstrbuf_addbuf(&outbuf, &dec);\n> +\t\tstrbuf_release(&dec);\n\n... release here, look somewhat pointless.  As you declared \"dec\" at\nthe outermost scope in this function, why not do the release at the\nplace everybody else is released/freed, at release_return: label?\n\n>  \t\tin = ep + 2;\n>  \t}\n>  \tstrbuf_addstr(&outbuf, in);\n> @@ -634,23 +638,24 @@ static int is_inbody_header(const struct mailinfo *mi,\n>  \n>  static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n>  {\n> -\tstruct strbuf *ret;\n> +\tstruct strbuf ret;\n\nDo the \"= STRBUF_INIT\" at the caller.\n\n> +\tint found_error = 0;\n>  \n>  \tswitch (mi->transfer_encoding) {\n>  \tcase TE_QP:\n> -\t\tret = decode_q_segment(line, 0);\n> +\t\tfound_error = decode_q_segment(&ret, line, 0);\n>  \t\tbreak;\n>  \tcase TE_BASE64:\n> -\t\tret = decode_b_segment(line);\n> +\t\tfound_error = decode_b_segment(&ret, line);\n>  \t\tbreak;\n>  \tcase TE_DONTCARE:\n>  \tdefault:\n>  \t\treturn;\n>  \t}\n>  \tstrbuf_reset(line);\n> -\tstrbuf_addbuf(line, ret);\n> -\tstrbuf_release(ret);\n> -\tfree(ret);\n> +\tstrbuf_addbuf(line, &ret);\n> +\tif (!found_error)\n> +\t\tstrbuf_release(&ret);\n>  }\n\nTHis is puzzling.  We add whatever is in ret to line, but release it\nonly when there is no error?  What happens when we did find error?\nThere does not seem to be any caller-callee contract on what the out\nparameter should contain upon an error, which is a good thing, so we\nshould release it unconditionally, no?\n\n\n\n"},{"id":"517864","messageId":"pull.1956.v3.git.git.1747066627330.gitgitgadget@gmail.com","threadId":"63419","inReplyTo":"pull.1956.v2.git.git.1746980097510.gitgitgadget@gmail.com","subject":"[PATCH v3] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-12T16:17:07Z","receivedAt":"2025-05-12T16:17:10Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored\nin dec will leak. Simply add strbuf_release and free(dec) will solve\nthis problem.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    decode_header: fix pointential memory leak if decode_header failed\n    \n    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n    dec will leak. Simply add strbuf_release and free(dec) will solve this\n    problem.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v3\nPull-Request: https://github.com/git/git/pull/1956\n\nRange-diff vs v2:\n\n 1:  90dc9b0d49b ! 1:  7f8ce472750 mailinfo: fix pointential memory leak if `decode_header` failed\n     @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)\n       \t\t\tgoto release_return;\n       \t\tcase 'b':\n      -\t\t\tdec = decode_b_segment(&piecebuf);\n     -+\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf))) {\n     ++\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf)))\n      +\t\t\t\tgoto release_return;\n     -+\t\t\t}\n       \t\t\tbreak;\n       \t\tcase 'q':\n      -\t\t\tdec = decode_q_segment(&piecebuf, 1);\n     -+\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {\n     ++\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1)))\n      +\t\t\t\tgoto release_return;\n     -+\t\t\t}\n       \t\t\tbreak;\n       \t\t}\n      -\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n      +\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n     -+\t\t\tstrbuf_release(&dec);\n       \t\t\tgoto release_return;\n      +\t\t}\n       \n     @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)\n       \t\tin = ep + 2;\n       \t}\n       \tstrbuf_addstr(&outbuf, in);\n     +@@ mailinfo.c: release_return:\n     + \tstrbuf_release(&outbuf);\n     + \tstrbuf_release(&charset_q);\n     + \tstrbuf_release(&piecebuf);\n     ++\tstrbuf_release(&dec);\n     + \n     + \tif (found_error)\n     + \t\tmi->input_error = -1;\n      @@ mailinfo.c: static int is_inbody_header(const struct mailinfo *mi,\n       \n       static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n       {\n      -\tstruct strbuf *ret;\n     -+\tstruct strbuf ret;\n     -+\tint found_error = 0;\n     ++\tstruct strbuf ret = STRBUF_INIT;\n       \n       \tswitch (mi->transfer_encoding) {\n       \tcase TE_QP:\n      -\t\tret = decode_q_segment(line, 0);\n     -+\t\tfound_error = decode_q_segment(&ret, line, 0);\n     ++\t\tdecode_q_segment(&ret, line, 0);\n       \t\tbreak;\n       \tcase TE_BASE64:\n      -\t\tret = decode_b_segment(line);\n     -+\t\tfound_error = decode_b_segment(&ret, line);\n     ++\t\tdecode_b_segment(&ret, line);\n       \t\tbreak;\n       \tcase TE_DONTCARE:\n       \tdefault:\n     @@ mailinfo.c: static int is_inbody_header(const struct mailinfo *mi,\n      -\tstrbuf_release(ret);\n      -\tfree(ret);\n      +\tstrbuf_addbuf(line, &ret);\n     -+\tif (!found_error)\n     -+\t\tstrbuf_release(&ret);\n     ++\tstrbuf_release(&ret);\n       }\n       \n       static inline int patchbreak(const struct strbuf *line)\n\n\n mailinfo.c | 39 ++++++++++++++++++++-------------------\n 1 file changed, 20 insertions(+), 19 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 7b001fa5dbd..9bda2e8170f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -381,11 +381,11 @@ static int is_format_patch_separator(const char *line, int len)\n \treturn !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));\n }\n \n-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n+static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n+\t\t\t    int rfc2047)\n {\n \tconst char *in = q_seg->buf;\n \tint c;\n-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n \tstrbuf_init(out, q_seg->len);\n \n \twhile ((c = *in++) != 0) {\n@@ -405,15 +405,14 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n \t\t\tc = 0x20;\n \t\tstrbuf_addch(out, c);\n \t}\n-\treturn out;\n+\treturn 0;\n }\n \n-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n+static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n {\n \t/* Decode in..ep, possibly in-place to ot */\n \tint c, pos = 0, acc = 0;\n \tconst char *in = b_seg->buf;\n-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n \tstrbuf_init(out, b_seg->len);\n \n \twhile ((c = *in++) != 0) {\n@@ -447,7 +446,7 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n \t\t\tbreak;\n \t\t}\n \t}\n-\treturn out;\n+\treturn 0;\n }\n \n static int convert_to_utf8(struct mailinfo *mi,\n@@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n static void decode_header(struct mailinfo *mi, struct strbuf *it)\n {\n \tchar *in, *ep, *cp;\n-\tstruct strbuf outbuf = STRBUF_INIT, *dec;\n+\tstruct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;\n \tstruct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;\n \tint found_error = 1; /* pessimism */\n \n@@ -530,18 +529,20 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n \t\tdefault:\n \t\t\tgoto release_return;\n \t\tcase 'b':\n-\t\t\tdec = decode_b_segment(&piecebuf);\n+\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf)))\n+\t\t\t\tgoto release_return;\n \t\t\tbreak;\n \t\tcase 'q':\n-\t\t\tdec = decode_q_segment(&piecebuf, 1);\n+\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1)))\n+\t\t\t\tgoto release_return;\n \t\t\tbreak;\n \t\t}\n-\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n+\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n \t\t\tgoto release_return;\n+\t\t}\n \n-\t\tstrbuf_addbuf(&outbuf, dec);\n-\t\tstrbuf_release(dec);\n-\t\tfree(dec);\n+\t\tstrbuf_addbuf(&outbuf, &dec);\n+\t\tstrbuf_release(&dec);\n \t\tin = ep + 2;\n \t}\n \tstrbuf_addstr(&outbuf, in);\n@@ -552,6 +553,7 @@ release_return:\n \tstrbuf_release(&outbuf);\n \tstrbuf_release(&charset_q);\n \tstrbuf_release(&piecebuf);\n+\tstrbuf_release(&dec);\n \n \tif (found_error)\n \t\tmi->input_error = -1;\n@@ -634,23 +636,22 @@ static int is_inbody_header(const struct mailinfo *mi,\n \n static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n {\n-\tstruct strbuf *ret;\n+\tstruct strbuf ret = STRBUF_INIT;\n \n \tswitch (mi->transfer_encoding) {\n \tcase TE_QP:\n-\t\tret = decode_q_segment(line, 0);\n+\t\tdecode_q_segment(&ret, line, 0);\n \t\tbreak;\n \tcase TE_BASE64:\n-\t\tret = decode_b_segment(line);\n+\t\tdecode_b_segment(&ret, line);\n \t\tbreak;\n \tcase TE_DONTCARE:\n \tdefault:\n \t\treturn;\n \t}\n \tstrbuf_reset(line);\n-\tstrbuf_addbuf(line, ret);\n-\tstrbuf_release(ret);\n-\tfree(ret);\n+\tstrbuf_addbuf(line, &ret);\n+\tstrbuf_release(&ret);\n }\n \n static inline int patchbreak(const struct strbuf *line)\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"517909","messageId":"xmqqbjrxsk07.fsf@gitster.g","threadId":"63419","inReplyTo":"pull.1956.v3.git.git.1747066627330.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-12T19:32:08Z","receivedAt":"2025-05-12T19:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> -static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n> +\t\t\t    int rfc2047)\n>  {\n>  \tconst char *in = q_seg->buf;\n>  \tint c;\n> -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n>  \tstrbuf_init(out, q_seg->len);\n\nThis strbuf_init() is still here?\n\n> +static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n>  {\n>  \t/* Decode in..ep, possibly in-place to ot */\n>  \tint c, pos = 0, acc = 0;\n>  \tconst char *in = b_seg->buf;\n> -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n>  \tstrbuf_init(out, b_seg->len);\n\nDitto.\n\n> @@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n>  static void decode_header(struct mailinfo *mi, struct strbuf *it)\n>  {\n>  \tchar *in, *ep, *cp;\n> -\tstruct strbuf outbuf = STRBUF_INIT, *dec;\n> +\tstruct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;\n\nInitializing this at the caller side is good.\n\n> @@ -530,18 +529,20 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n>  \t\tdefault:\n>  \t\t\tgoto release_return;\n>  \t\tcase 'b':\n> -\t\t\tdec = decode_b_segment(&piecebuf);\n> +\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf)))\n> +\t\t\t\tgoto release_return;\n>  \t\t\tbreak;\n>  \t\tcase 'q':\n> -\t\t\tdec = decode_q_segment(&piecebuf, 1);\n> +\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1)))\n> +\t\t\t\tgoto release_return;\n>  \t\t\tbreak;\n>  \t\t}\n> -\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n> +\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n>  \t\t\tgoto release_return;\n> +\t\t}\n\nDon't enclose a single statement block inside {braces}.\n\n\nThanks.\n"},{"id":"517922","messageId":"pull.1956.v4.git.git.1747104551204.gitgitgadget@gmail.com","threadId":"63419","inReplyTo":"pull.1956.v3.git.git.1747066627330.gitgitgadget@gmail.com","subject":"[PATCH v4] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-13T02:49:10Z","receivedAt":"2025-05-13T02:49:14Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored\nin dec will leak. Simply add strbuf_release and free(dec) will solve\nthis problem.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    decode_header: fix pointential memory leak if decode_header failed\n    \n    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored in\n    dec will leak. Simply add strbuf_release and free(dec) will solve this\n    problem.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v4\nPull-Request: https://github.com/git/git/pull/1956\n\nRange-diff vs v3:\n\n 1:  7f8ce472750 ! 1:  d00e805c5c5 mailinfo: fix pointential memory leak if `decode_header` failed\n     @@ mailinfo.c: static int is_format_patch_separator(const char *line, int len)\n       \tconst char *in = q_seg->buf;\n       \tint c;\n      -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n     - \tstrbuf_init(out, q_seg->len);\n     +-\tstrbuf_init(out, q_seg->len);\n     ++\tstrbuf_grow(out, q_seg->len);\n       \n       \twhile ((c = *in++) != 0) {\n     + \t\tif (c == '=') {\n      @@ mailinfo.c: static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n       \t\t\tc = 0x20;\n       \t\tstrbuf_addch(out, c);\n     @@ mailinfo.c: static struct strbuf *decode_q_segment(const struct strbuf *q_seg, i\n       \tint c, pos = 0, acc = 0;\n       \tconst char *in = b_seg->buf;\n      -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n     - \tstrbuf_init(out, b_seg->len);\n     +-\tstrbuf_init(out, b_seg->len);\n     ++\tstrbuf_grow(out, b_seg->len);\n       \n       \twhile ((c = *in++) != 0) {\n     + \t\tif (c == '+')\n      @@ mailinfo.c: static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n       \t\t\tbreak;\n       \t\t}\n     @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)\n       \t\t\tbreak;\n       \t\t}\n      -\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n     -+\t\tif (convert_to_utf8(mi, &dec, charset_q.buf)) {\n     ++\t\tif (convert_to_utf8(mi, &dec, charset_q.buf))\n       \t\t\tgoto release_return;\n     -+\t\t}\n       \n      -\t\tstrbuf_addbuf(&outbuf, dec);\n      -\t\tstrbuf_release(dec);\n\n\n mailinfo.c | 42 +++++++++++++++++++++---------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 7b001fa5dbd..ee4597da6be 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -381,12 +381,12 @@ static int is_format_patch_separator(const char *line, int len)\n \treturn !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));\n }\n \n-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n+static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n+\t\t\t    int rfc2047)\n {\n \tconst char *in = q_seg->buf;\n \tint c;\n-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n-\tstrbuf_init(out, q_seg->len);\n+\tstrbuf_grow(out, q_seg->len);\n \n \twhile ((c = *in++) != 0) {\n \t\tif (c == '=') {\n@@ -405,16 +405,15 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n \t\t\tc = 0x20;\n \t\tstrbuf_addch(out, c);\n \t}\n-\treturn out;\n+\treturn 0;\n }\n \n-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n+static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n {\n \t/* Decode in..ep, possibly in-place to ot */\n \tint c, pos = 0, acc = 0;\n \tconst char *in = b_seg->buf;\n-\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n-\tstrbuf_init(out, b_seg->len);\n+\tstrbuf_grow(out, b_seg->len);\n \n \twhile ((c = *in++) != 0) {\n \t\tif (c == '+')\n@@ -447,7 +446,7 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n \t\t\tbreak;\n \t\t}\n \t}\n-\treturn out;\n+\treturn 0;\n }\n \n static int convert_to_utf8(struct mailinfo *mi,\n@@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n static void decode_header(struct mailinfo *mi, struct strbuf *it)\n {\n \tchar *in, *ep, *cp;\n-\tstruct strbuf outbuf = STRBUF_INIT, *dec;\n+\tstruct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;\n \tstruct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;\n \tint found_error = 1; /* pessimism */\n \n@@ -530,18 +529,19 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n \t\tdefault:\n \t\t\tgoto release_return;\n \t\tcase 'b':\n-\t\t\tdec = decode_b_segment(&piecebuf);\n+\t\t\tif ((found_error = decode_b_segment(&dec, &piecebuf)))\n+\t\t\t\tgoto release_return;\n \t\t\tbreak;\n \t\tcase 'q':\n-\t\t\tdec = decode_q_segment(&piecebuf, 1);\n+\t\t\tif ((found_error = decode_q_segment(&dec, &piecebuf, 1)))\n+\t\t\t\tgoto release_return;\n \t\t\tbreak;\n \t\t}\n-\t\tif (convert_to_utf8(mi, dec, charset_q.buf))\n+\t\tif (convert_to_utf8(mi, &dec, charset_q.buf))\n \t\t\tgoto release_return;\n \n-\t\tstrbuf_addbuf(&outbuf, dec);\n-\t\tstrbuf_release(dec);\n-\t\tfree(dec);\n+\t\tstrbuf_addbuf(&outbuf, &dec);\n+\t\tstrbuf_release(&dec);\n \t\tin = ep + 2;\n \t}\n \tstrbuf_addstr(&outbuf, in);\n@@ -552,6 +552,7 @@ release_return:\n \tstrbuf_release(&outbuf);\n \tstrbuf_release(&charset_q);\n \tstrbuf_release(&piecebuf);\n+\tstrbuf_release(&dec);\n \n \tif (found_error)\n \t\tmi->input_error = -1;\n@@ -634,23 +635,22 @@ static int is_inbody_header(const struct mailinfo *mi,\n \n static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n {\n-\tstruct strbuf *ret;\n+\tstruct strbuf ret = STRBUF_INIT;\n \n \tswitch (mi->transfer_encoding) {\n \tcase TE_QP:\n-\t\tret = decode_q_segment(line, 0);\n+\t\tdecode_q_segment(&ret, line, 0);\n \t\tbreak;\n \tcase TE_BASE64:\n-\t\tret = decode_b_segment(line);\n+\t\tdecode_b_segment(&ret, line);\n \t\tbreak;\n \tcase TE_DONTCARE:\n \tdefault:\n \t\treturn;\n \t}\n \tstrbuf_reset(line);\n-\tstrbuf_addbuf(line, ret);\n-\tstrbuf_release(ret);\n-\tfree(ret);\n+\tstrbuf_addbuf(line, &ret);\n+\tstrbuf_release(&ret);\n }\n \n static inline int patchbreak(const struct strbuf *line)\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"517945","messageId":"xmqq4ixoiqhu.fsf@gitster.g","threadId":"63419","inReplyTo":"pull.1956.v4.git.git.1747104551204.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-13T13:34:21Z","receivedAt":"2025-05-13T13:34:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> -static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n> +\t\t\t    int rfc2047)\n>  {\n>  \tconst char *in = q_seg->buf;\n>  \tint c;\n> -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n> -\tstrbuf_init(out, q_seg->len);\n> +\tstrbuf_grow(out, q_seg->len);\n\nCall to grow(), while it does not hurt correctness, would not be\nnecessary here, but let's take this code as-is.  As the result of Q\nencoding (and B encoding as well) always is longer than the decoded\nresult, strbuf_grow(out, q_seg->len) would always be over-allocating,\nbut it would not hurt too much.\n\n> -static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n> +static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)\n>  {\n>  \t/* Decode in..ep, possibly in-place to ot */\n>  \tint c, pos = 0, acc = 0;\n>  \tconst char *in = b_seg->buf;\n> -\tstruct strbuf *out = xmalloc(sizeof(struct strbuf));\n> -\tstrbuf_init(out, b_seg->len);\n> +\tstrbuf_grow(out, b_seg->len);\n\nDitto.\n\n>  \tchar *in, *ep, *cp;\n> -\tstruct strbuf outbuf = STRBUF_INIT, *dec;\n> +\tstruct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;\n>  \tstruct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;\n>  \tint found_error = 1; /* pessimism */\n\nThe remainder of the patch also looks good.\n\nThanks.\n"},{"id":"517946","messageId":"9DE4050D-B036-4477-93B0-FF738A6F5C3C@smail.nju.edu.cn","threadId":"63419","inReplyTo":"xmqq4ixoiqhu.fsf@gitster.g","subject":"Re: [PATCH v4] mailinfo: fix pointential memory leak if `decode_header` failed","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-13T13:42:29Z","receivedAt":"2025-05-13T13:42:57Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano <gitster@pobox.com> 写道：\n> \n> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> -static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)\n>> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,\n>> +    int rfc2047)\n>> {\n>> const char *in = q_seg->buf;\n>> int c;\n>> - struct strbuf *out = xmalloc(sizeof(struct strbuf));\n>> - strbuf_init(out, q_seg->len);\n>> + strbuf_grow(out, q_seg->len);\n> \n> Call to grow(), while it does not hurt correctness, would not be\n> necessary here, but let's take this code as-is.  As the result of Q\n> encoding (and B encoding as well) always is longer than the decoded\n> result, strbuf_grow(out, q_seg->len) would always be over-allocating,\n> but it would not hurt too much.\n\nThanks for your advice. Though I think `strbuf_init(out, q_seg->len)` also \ncall `strbuf_grow` to reserve `q_seg->len` size memory for `out`. \n`strbuf_grow` here prevent multiple realloc operations that might occur\nwhen calling `strbuf_addch`."}]}