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

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

[Lidong Yan via GitGitGadget]

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.

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.

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);
 			goto release_return;
+		}
 
 		strbuf_addbuf(&outbuf, dec);
 		strbuf_release(dec);

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

[lidongyan]

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

[PATCH v2] mailinfo: fix pointential memory leak if `decode_header` failed

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored
in dec will leak. Simply add strbuf_release and free(dec) will solve
this problem.

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v2
Pull-Request: https://github.com/git/git/pull/1956

Range-diff vs v1:

 1:  81fdfb94315 ! 1:  90dc9b0d49b decode_header: fix pointential memory leak if decode_header failed
     @@ Metadata
      Author: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## Commit message ##
     -    decode_header: fix pointential memory leak if decode_header failed
     +    mailinfo: fix pointential memory leak if `decode_header` failed
      
     -    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored
     +    In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored
          in dec will leak. Simply add strbuf_release and free(dec) will solve
          this problem.
      
          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## mailinfo.c ##
     +@@ mailinfo.c: static int is_format_patch_separator(const char *line, int len)
     + 	return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
     + }
     + 
     +-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
     ++static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
     ++			    int rfc2047)
     + {
     + 	const char *in = q_seg->buf;
     + 	int c;
     +-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
     + 	strbuf_init(out, q_seg->len);
     + 
     + 	while ((c = *in++) != 0) {
     +@@ mailinfo.c: static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
     + 			c = 0x20;
     + 		strbuf_addch(out, c);
     + 	}
     +-	return out;
     ++	return 0;
     + }
     + 
     +-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
     ++static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
     + {
     + 	/* Decode in..ep, possibly in-place to ot */
     + 	int c, pos = 0, acc = 0;
     + 	const char *in = b_seg->buf;
     +-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
     + 	strbuf_init(out, b_seg->len);
     + 
     + 	while ((c = *in++) != 0) {
     +@@ mailinfo.c: static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
     + 			break;
     + 		}
     + 	}
     +-	return out;
     ++	return 0;
     + }
     + 
     + static int convert_to_utf8(struct mailinfo *mi,
     +@@ mailinfo.c: static int convert_to_utf8(struct mailinfo *mi,
     + static void decode_header(struct mailinfo *mi, struct strbuf *it)
     + {
     + 	char *in, *ep, *cp;
     +-	struct strbuf outbuf = STRBUF_INIT, *dec;
     ++	struct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;
     + 	struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;
     + 	int found_error = 1; /* pessimism */
     + 
      @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)
     - 			dec = decode_q_segment(&piecebuf, 1);
     + 		default:
     + 			goto release_return;
     + 		case 'b':
     +-			dec = decode_b_segment(&piecebuf);
     ++			if ((found_error = decode_b_segment(&dec, &piecebuf))) {
     ++				goto release_return;
     ++			}
     + 			break;
     + 		case 'q':
     +-			dec = decode_q_segment(&piecebuf, 1);
     ++			if ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {
     ++				goto release_return;
     ++			}
       			break;
       		}
      -		if (convert_to_utf8(mi, dec, charset_q.buf))
     -+		if (convert_to_utf8(mi, dec, charset_q.buf)) {
     -+			strbuf_release(dec);
     -+			free(dec);
     ++		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
     ++			strbuf_release(&dec);
       			goto release_return;
      +		}
       
     - 		strbuf_addbuf(&outbuf, dec);
     - 		strbuf_release(dec);
     +-		strbuf_addbuf(&outbuf, dec);
     +-		strbuf_release(dec);
     +-		free(dec);
     ++		strbuf_addbuf(&outbuf, &dec);
     ++		strbuf_release(&dec);
     + 		in = ep + 2;
     + 	}
     + 	strbuf_addstr(&outbuf, in);
     +@@ mailinfo.c: static int is_inbody_header(const struct mailinfo *mi,
     + 
     + static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
     + {
     +-	struct strbuf *ret;
     ++	struct strbuf ret;
     ++	int found_error = 0;
     + 
     + 	switch (mi->transfer_encoding) {
     + 	case TE_QP:
     +-		ret = decode_q_segment(line, 0);
     ++		found_error = decode_q_segment(&ret, line, 0);
     + 		break;
     + 	case TE_BASE64:
     +-		ret = decode_b_segment(line);
     ++		found_error = decode_b_segment(&ret, line);
     + 		break;
     + 	case TE_DONTCARE:
     + 	default:
     + 		return;
     + 	}
     + 	strbuf_reset(line);
     +-	strbuf_addbuf(line, ret);
     +-	strbuf_release(ret);
     +-	free(ret);
     ++	strbuf_addbuf(line, &ret);
     ++	if (!found_error)
     ++		strbuf_release(&ret);
     + }
     + 
     + static inline int patchbreak(const struct strbuf *line)


 mailinfo.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 7b001fa5dbd..11ec3914ce4 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -381,11 +381,11 @@ static int is_format_patch_separator(const char *line, int len)
 	return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
 }
 
-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
+static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
+			    int rfc2047)
 {
 	const char *in = q_seg->buf;
 	int c;
-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
 	strbuf_init(out, q_seg->len);
 
 	while ((c = *in++) != 0) {
@@ -405,15 +405,14 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
 			c = 0x20;
 		strbuf_addch(out, c);
 	}
-	return out;
+	return 0;
 }
 
-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
+static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
 {
 	/* Decode in..ep, possibly in-place to ot */
 	int c, pos = 0, acc = 0;
 	const char *in = b_seg->buf;
-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
 	strbuf_init(out, b_seg->len);
 
 	while ((c = *in++) != 0) {
@@ -447,7 +446,7 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
 			break;
 		}
 	}
-	return out;
+	return 0;
 }
 
 static int convert_to_utf8(struct mailinfo *mi,
@@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,
 static void decode_header(struct mailinfo *mi, struct strbuf *it)
 {
 	char *in, *ep, *cp;
-	struct strbuf outbuf = STRBUF_INIT, *dec;
+	struct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;
 	struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;
 	int found_error = 1; /* pessimism */
 
@@ -530,18 +529,23 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
 		default:
 			goto release_return;
 		case 'b':
-			dec = decode_b_segment(&piecebuf);
+			if ((found_error = decode_b_segment(&dec, &piecebuf))) {
+				goto release_return;
+			}
 			break;
 		case 'q':
-			dec = decode_q_segment(&piecebuf, 1);
+			if ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {
+				goto release_return;
+			}
 			break;
 		}
-		if (convert_to_utf8(mi, dec, charset_q.buf))
+		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
+			strbuf_release(&dec);
 			goto release_return;
+		}
 
-		strbuf_addbuf(&outbuf, dec);
-		strbuf_release(dec);
-		free(dec);
+		strbuf_addbuf(&outbuf, &dec);
+		strbuf_release(&dec);
 		in = ep + 2;
 	}
 	strbuf_addstr(&outbuf, in);
@@ -634,23 +638,24 @@ static int is_inbody_header(const struct mailinfo *mi,
 
 static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 {
-	struct strbuf *ret;
+	struct strbuf ret;
+	int found_error = 0;
 
 	switch (mi->transfer_encoding) {
 	case TE_QP:
-		ret = decode_q_segment(line, 0);
+		found_error = decode_q_segment(&ret, line, 0);
 		break;
 	case TE_BASE64:
-		ret = decode_b_segment(line);
+		found_error = decode_b_segment(&ret, line);
 		break;
 	case TE_DONTCARE:
 	default:
 		return;
 	}
 	strbuf_reset(line);
-	strbuf_addbuf(line, ret);
-	strbuf_release(ret);
-	free(ret);
+	strbuf_addbuf(line, &ret);
+	if (!found_error)
+		strbuf_release(&ret);
 }
 
 static inline int patchbreak(const struct strbuf *line)

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
-- 
gitgitgadget

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

[Junio C Hamano]

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

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored
> in dec will leak. Simply add strbuf_release and free(dec) will solve
> this problem.
>
> 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.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v2
> Pull-Request: https://github.com/git/git/pull/1956
>
> Range-diff vs v1:
>
>  1:  81fdfb94315 ! 1:  90dc9b0d49b decode_header: fix pointential memory leak if decode_header failed
>      @@ Metadata
>       Author: Lidong Yan <502024330056@smail.nju.edu.cn>
>       
>        ## Commit message ##
>      -    decode_header: fix pointential memory leak if decode_header failed
>      +    mailinfo: fix pointential memory leak if `decode_header` failed
>       
>      -    In mailinfo.c line 539, if convert_to_utf8 failed, the strbuf stored
>      +    In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored
>           in dec will leak. Simply add strbuf_release and free(dec) will solve
>           this problem.

Much better.

> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
> +			    int rfc2047)
>  {
>  	const char *in = q_seg->buf;
>  	int c;
> -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
>  	strbuf_init(out, q_seg->len);

Don't let the caller pass in an uninitialized thing and force the
callee to initialize it.  Drop this strbuf_init(), and make the
caller always do:

	struct strbuf dec = STRBUF_INIT;

	...
		decode_q_segment(&dec, ...);

instead.  That makes the division of labor easier to see (e.g., what
if the caller had a code path that never calls decode_x_segment()
before it has to return?  it might want to add something to dec
itself so that it can base its behaviour always on what is in dec,
or at the end it may just be able to strbuf_release(&dec) without
having to remember if it called decode_x_segment().  Which means it
is more convenient for it to always assume that dec is initialized
whether it called decode_x_segment() or not).

> -static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
> +static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
>  {
>  	/* Decode in..ep, possibly in-place to ot */
>  	int c, pos = 0, acc = 0;
>  	const char *in = b_seg->buf;
> -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
>  	strbuf_init(out, b_seg->len);

Ditto.

> @@ -530,18 +529,23 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
>  		default:
>  			goto release_return;
>  		case 'b':
> -			dec = decode_b_segment(&piecebuf);
> +			if ((found_error = decode_b_segment(&dec, &piecebuf))) {
> +				goto release_return;
> +			}

Don't enclose a single statement block inside {braces}.

>  			break;
>  		case 'q':
> -			dec = decode_q_segment(&piecebuf, 1);
> +			if ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {
> +				goto release_return;
> +			}

Ditto.

>  			break;

Just a mental note (i.e., not anything wrong in the posted patch).
Even though the caller is prepared to see decode_x_segment() to
notice and report an error, the implementation just does not bother,
and mostly skips a garbage in the input.  Outside the topic of this
series, we may want to consider allowing the user to say "be strict
and barf when encoded contents are broken".

>  		}
> -		if (convert_to_utf8(mi, dec, charset_q.buf))
> +		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
> +			strbuf_release(&dec);
>  			goto release_return;
> +		}

This, together with ...

> -		strbuf_addbuf(&outbuf, dec);
> -		strbuf_release(dec);
> -		free(dec);
> +		strbuf_addbuf(&outbuf, &dec);
> +		strbuf_release(&dec);

... release here, look somewhat pointless.  As you declared "dec" at
the outermost scope in this function, why not do the release at the
place everybody else is released/freed, at release_return: label?

>  		in = ep + 2;
>  	}
>  	strbuf_addstr(&outbuf, in);
> @@ -634,23 +638,24 @@ static int is_inbody_header(const struct mailinfo *mi,
>  
>  static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
>  {
> -	struct strbuf *ret;
> +	struct strbuf ret;

Do the "= STRBUF_INIT" at the caller.

> +	int found_error = 0;
>  
>  	switch (mi->transfer_encoding) {
>  	case TE_QP:
> -		ret = decode_q_segment(line, 0);
> +		found_error = decode_q_segment(&ret, line, 0);
>  		break;
>  	case TE_BASE64:
> -		ret = decode_b_segment(line);
> +		found_error = decode_b_segment(&ret, line);
>  		break;
>  	case TE_DONTCARE:
>  	default:
>  		return;
>  	}
>  	strbuf_reset(line);
> -	strbuf_addbuf(line, ret);
> -	strbuf_release(ret);
> -	free(ret);
> +	strbuf_addbuf(line, &ret);
> +	if (!found_error)
> +		strbuf_release(&ret);
>  }

THis is puzzling.  We add whatever is in ret to line, but release it
only when there is no error?  What happens when we did find error?
There does not seem to be any caller-callee contract on what the out
parameter should contain upon an error, which is a good thing, so we
should release it unconditionally, no?

[PATCH v3] mailinfo: fix pointential memory leak if `decode_header` failed

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored
in dec will leak. Simply add strbuf_release and free(dec) will solve
this problem.

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v3
Pull-Request: https://github.com/git/git/pull/1956

Range-diff vs v2:

 1:  90dc9b0d49b ! 1:  7f8ce472750 mailinfo: fix pointential memory leak if `decode_header` failed
     @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)
       			goto release_return;
       		case 'b':
      -			dec = decode_b_segment(&piecebuf);
     -+			if ((found_error = decode_b_segment(&dec, &piecebuf))) {
     ++			if ((found_error = decode_b_segment(&dec, &piecebuf)))
      +				goto release_return;
     -+			}
       			break;
       		case 'q':
      -			dec = decode_q_segment(&piecebuf, 1);
     -+			if ((found_error = decode_q_segment(&dec, &piecebuf, 1))) {
     ++			if ((found_error = decode_q_segment(&dec, &piecebuf, 1)))
      +				goto release_return;
     -+			}
       			break;
       		}
      -		if (convert_to_utf8(mi, dec, charset_q.buf))
      +		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
     -+			strbuf_release(&dec);
       			goto release_return;
      +		}
       
     @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)
       		in = ep + 2;
       	}
       	strbuf_addstr(&outbuf, in);
     +@@ mailinfo.c: release_return:
     + 	strbuf_release(&outbuf);
     + 	strbuf_release(&charset_q);
     + 	strbuf_release(&piecebuf);
     ++	strbuf_release(&dec);
     + 
     + 	if (found_error)
     + 		mi->input_error = -1;
      @@ mailinfo.c: static int is_inbody_header(const struct mailinfo *mi,
       
       static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
       {
      -	struct strbuf *ret;
     -+	struct strbuf ret;
     -+	int found_error = 0;
     ++	struct strbuf ret = STRBUF_INIT;
       
       	switch (mi->transfer_encoding) {
       	case TE_QP:
      -		ret = decode_q_segment(line, 0);
     -+		found_error = decode_q_segment(&ret, line, 0);
     ++		decode_q_segment(&ret, line, 0);
       		break;
       	case TE_BASE64:
      -		ret = decode_b_segment(line);
     -+		found_error = decode_b_segment(&ret, line);
     ++		decode_b_segment(&ret, line);
       		break;
       	case TE_DONTCARE:
       	default:
     @@ mailinfo.c: static int is_inbody_header(const struct mailinfo *mi,
      -	strbuf_release(ret);
      -	free(ret);
      +	strbuf_addbuf(line, &ret);
     -+	if (!found_error)
     -+		strbuf_release(&ret);
     ++	strbuf_release(&ret);
       }
       
       static inline int patchbreak(const struct strbuf *line)


 mailinfo.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 7b001fa5dbd..9bda2e8170f 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -381,11 +381,11 @@ static int is_format_patch_separator(const char *line, int len)
 	return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
 }
 
-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
+static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
+			    int rfc2047)
 {
 	const char *in = q_seg->buf;
 	int c;
-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
 	strbuf_init(out, q_seg->len);
 
 	while ((c = *in++) != 0) {
@@ -405,15 +405,14 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
 			c = 0x20;
 		strbuf_addch(out, c);
 	}
-	return out;
+	return 0;
 }
 
-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
+static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
 {
 	/* Decode in..ep, possibly in-place to ot */
 	int c, pos = 0, acc = 0;
 	const char *in = b_seg->buf;
-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
 	strbuf_init(out, b_seg->len);
 
 	while ((c = *in++) != 0) {
@@ -447,7 +446,7 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
 			break;
 		}
 	}
-	return out;
+	return 0;
 }
 
 static int convert_to_utf8(struct mailinfo *mi,
@@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,
 static void decode_header(struct mailinfo *mi, struct strbuf *it)
 {
 	char *in, *ep, *cp;
-	struct strbuf outbuf = STRBUF_INIT, *dec;
+	struct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;
 	struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;
 	int found_error = 1; /* pessimism */
 
@@ -530,18 +529,20 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
 		default:
 			goto release_return;
 		case 'b':
-			dec = decode_b_segment(&piecebuf);
+			if ((found_error = decode_b_segment(&dec, &piecebuf)))
+				goto release_return;
 			break;
 		case 'q':
-			dec = decode_q_segment(&piecebuf, 1);
+			if ((found_error = decode_q_segment(&dec, &piecebuf, 1)))
+				goto release_return;
 			break;
 		}
-		if (convert_to_utf8(mi, dec, charset_q.buf))
+		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
 			goto release_return;
+		}
 
-		strbuf_addbuf(&outbuf, dec);
-		strbuf_release(dec);
-		free(dec);
+		strbuf_addbuf(&outbuf, &dec);
+		strbuf_release(&dec);
 		in = ep + 2;
 	}
 	strbuf_addstr(&outbuf, in);
@@ -552,6 +553,7 @@ release_return:
 	strbuf_release(&outbuf);
 	strbuf_release(&charset_q);
 	strbuf_release(&piecebuf);
+	strbuf_release(&dec);
 
 	if (found_error)
 		mi->input_error = -1;
@@ -634,23 +636,22 @@ static int is_inbody_header(const struct mailinfo *mi,
 
 static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 {
-	struct strbuf *ret;
+	struct strbuf ret = STRBUF_INIT;
 
 	switch (mi->transfer_encoding) {
 	case TE_QP:
-		ret = decode_q_segment(line, 0);
+		decode_q_segment(&ret, line, 0);
 		break;
 	case TE_BASE64:
-		ret = decode_b_segment(line);
+		decode_b_segment(&ret, line);
 		break;
 	case TE_DONTCARE:
 	default:
 		return;
 	}
 	strbuf_reset(line);
-	strbuf_addbuf(line, ret);
-	strbuf_release(ret);
-	free(ret);
+	strbuf_addbuf(line, &ret);
+	strbuf_release(&ret);
 }
 
 static inline int patchbreak(const struct strbuf *line)

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
-- 
gitgitgadget

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

[Junio C Hamano]

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

> -static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
> +			    int rfc2047)
>  {
>  	const char *in = q_seg->buf;
>  	int c;
> -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
>  	strbuf_init(out, q_seg->len);

This strbuf_init() is still here?

> +static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
>  {
>  	/* Decode in..ep, possibly in-place to ot */
>  	int c, pos = 0, acc = 0;
>  	const char *in = b_seg->buf;
> -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
>  	strbuf_init(out, b_seg->len);

Ditto.

> @@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,
>  static void decode_header(struct mailinfo *mi, struct strbuf *it)
>  {
>  	char *in, *ep, *cp;
> -	struct strbuf outbuf = STRBUF_INIT, *dec;
> +	struct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;

Initializing this at the caller side is good.

> @@ -530,18 +529,20 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
>  		default:
>  			goto release_return;
>  		case 'b':
> -			dec = decode_b_segment(&piecebuf);
> +			if ((found_error = decode_b_segment(&dec, &piecebuf)))
> +				goto release_return;
>  			break;
>  		case 'q':
> -			dec = decode_q_segment(&piecebuf, 1);
> +			if ((found_error = decode_q_segment(&dec, &piecebuf, 1)))
> +				goto release_return;
>  			break;
>  		}
> -		if (convert_to_utf8(mi, dec, charset_q.buf))
> +		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
>  			goto release_return;
> +		}

Don't enclose a single statement block inside {braces}.


Thanks.

[PATCH v4] mailinfo: fix pointential memory leak if `decode_header` failed

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In mailinfo.c:decode_header, if convert_to_utf8 failed, the strbuf stored
in dec will leak. Simply add strbuf_release and free(dec) will solve
this problem.

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1956%2Fbrandb97%2Ffix-mailinfo-decode-header-leak-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1956/brandb97/fix-mailinfo-decode-header-leak-v4
Pull-Request: https://github.com/git/git/pull/1956

Range-diff vs v3:

 1:  7f8ce472750 ! 1:  d00e805c5c5 mailinfo: fix pointential memory leak if `decode_header` failed
     @@ mailinfo.c: static int is_format_patch_separator(const char *line, int len)
       	const char *in = q_seg->buf;
       	int c;
      -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
     - 	strbuf_init(out, q_seg->len);
     +-	strbuf_init(out, q_seg->len);
     ++	strbuf_grow(out, q_seg->len);
       
       	while ((c = *in++) != 0) {
     + 		if (c == '=') {
      @@ mailinfo.c: static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
       			c = 0x20;
       		strbuf_addch(out, c);
     @@ mailinfo.c: static struct strbuf *decode_q_segment(const struct strbuf *q_seg, i
       	int c, pos = 0, acc = 0;
       	const char *in = b_seg->buf;
      -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
     - 	strbuf_init(out, b_seg->len);
     +-	strbuf_init(out, b_seg->len);
     ++	strbuf_grow(out, b_seg->len);
       
       	while ((c = *in++) != 0) {
     + 		if (c == '+')
      @@ mailinfo.c: static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
       			break;
       		}
     @@ mailinfo.c: static void decode_header(struct mailinfo *mi, struct strbuf *it)
       			break;
       		}
      -		if (convert_to_utf8(mi, dec, charset_q.buf))
     -+		if (convert_to_utf8(mi, &dec, charset_q.buf)) {
     ++		if (convert_to_utf8(mi, &dec, charset_q.buf))
       			goto release_return;
     -+		}
       
      -		strbuf_addbuf(&outbuf, dec);
      -		strbuf_release(dec);


 mailinfo.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 7b001fa5dbd..ee4597da6be 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -381,12 +381,12 @@ static int is_format_patch_separator(const char *line, int len)
 	return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
 }
 
-static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
+static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
+			    int rfc2047)
 {
 	const char *in = q_seg->buf;
 	int c;
-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
-	strbuf_init(out, q_seg->len);
+	strbuf_grow(out, q_seg->len);
 
 	while ((c = *in++) != 0) {
 		if (c == '=') {
@@ -405,16 +405,15 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
 			c = 0x20;
 		strbuf_addch(out, c);
 	}
-	return out;
+	return 0;
 }
 
-static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
+static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
 {
 	/* Decode in..ep, possibly in-place to ot */
 	int c, pos = 0, acc = 0;
 	const char *in = b_seg->buf;
-	struct strbuf *out = xmalloc(sizeof(struct strbuf));
-	strbuf_init(out, b_seg->len);
+	strbuf_grow(out, b_seg->len);
 
 	while ((c = *in++) != 0) {
 		if (c == '+')
@@ -447,7 +446,7 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
 			break;
 		}
 	}
-	return out;
+	return 0;
 }
 
 static int convert_to_utf8(struct mailinfo *mi,
@@ -475,7 +474,7 @@ static int convert_to_utf8(struct mailinfo *mi,
 static void decode_header(struct mailinfo *mi, struct strbuf *it)
 {
 	char *in, *ep, *cp;
-	struct strbuf outbuf = STRBUF_INIT, *dec;
+	struct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;
 	struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;
 	int found_error = 1; /* pessimism */
 
@@ -530,18 +529,19 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
 		default:
 			goto release_return;
 		case 'b':
-			dec = decode_b_segment(&piecebuf);
+			if ((found_error = decode_b_segment(&dec, &piecebuf)))
+				goto release_return;
 			break;
 		case 'q':
-			dec = decode_q_segment(&piecebuf, 1);
+			if ((found_error = decode_q_segment(&dec, &piecebuf, 1)))
+				goto release_return;
 			break;
 		}
-		if (convert_to_utf8(mi, dec, charset_q.buf))
+		if (convert_to_utf8(mi, &dec, charset_q.buf))
 			goto release_return;
 
-		strbuf_addbuf(&outbuf, dec);
-		strbuf_release(dec);
-		free(dec);
+		strbuf_addbuf(&outbuf, &dec);
+		strbuf_release(&dec);
 		in = ep + 2;
 	}
 	strbuf_addstr(&outbuf, in);
@@ -552,6 +552,7 @@ release_return:
 	strbuf_release(&outbuf);
 	strbuf_release(&charset_q);
 	strbuf_release(&piecebuf);
+	strbuf_release(&dec);
 
 	if (found_error)
 		mi->input_error = -1;
@@ -634,23 +635,22 @@ static int is_inbody_header(const struct mailinfo *mi,
 
 static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
 {
-	struct strbuf *ret;
+	struct strbuf ret = STRBUF_INIT;
 
 	switch (mi->transfer_encoding) {
 	case TE_QP:
-		ret = decode_q_segment(line, 0);
+		decode_q_segment(&ret, line, 0);
 		break;
 	case TE_BASE64:
-		ret = decode_b_segment(line);
+		decode_b_segment(&ret, line);
 		break;
 	case TE_DONTCARE:
 	default:
 		return;
 	}
 	strbuf_reset(line);
-	strbuf_addbuf(line, ret);
-	strbuf_release(ret);
-	free(ret);
+	strbuf_addbuf(line, &ret);
+	strbuf_release(&ret);
 }
 
 static inline int patchbreak(const struct strbuf *line)

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
-- 
gitgitgadget

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

[Junio C Hamano]

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

> -static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
> +			    int rfc2047)
>  {
>  	const char *in = q_seg->buf;
>  	int c;
> -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
> -	strbuf_init(out, q_seg->len);
> +	strbuf_grow(out, q_seg->len);

Call to grow(), while it does not hurt correctness, would not be
necessary here, but let's take this code as-is.  As the result of Q
encoding (and B encoding as well) always is longer than the decoded
result, strbuf_grow(out, q_seg->len) would always be over-allocating,
but it would not hurt too much.

> -static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
> +static int decode_b_segment(struct strbuf *out, const struct strbuf *b_seg)
>  {
>  	/* Decode in..ep, possibly in-place to ot */
>  	int c, pos = 0, acc = 0;
>  	const char *in = b_seg->buf;
> -	struct strbuf *out = xmalloc(sizeof(struct strbuf));
> -	strbuf_init(out, b_seg->len);
> +	strbuf_grow(out, b_seg->len);

Ditto.

>  	char *in, *ep, *cp;
> -	struct strbuf outbuf = STRBUF_INIT, *dec;
> +	struct strbuf outbuf = STRBUF_INIT, dec = STRBUF_INIT;
>  	struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT;
>  	int found_error = 1; /* pessimism */

The remainder of the patch also looks good.

Thanks.

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

[lidongyan]

Junio C Hamano <gitster@pobox.com> 写道：
> 
> "Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> -static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
>> +static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
>> +    int rfc2047)
>> {
>> const char *in = q_seg->buf;
>> int c;
>> - struct strbuf *out = xmalloc(sizeof(struct strbuf));
>> - strbuf_init(out, q_seg->len);
>> + strbuf_grow(out, q_seg->len);
> 
> Call to grow(), while it does not hurt correctness, would not be
> necessary here, but let's take this code as-is.  As the result of Q
> encoding (and B encoding as well) always is longer than the decoded
> result, strbuf_grow(out, q_seg->len) would always be over-allocating,
> but it would not hurt too much.

Thanks for your advice. Though I think `strbuf_init(out, q_seg->len)` also 
call `strbuf_grow` to reserve `q_seg->len` size memory for `out`. 
`strbuf_grow` here prevent multiple realloc operations that might occur
when calling `strbuf_addch`.