Re: [PATCH] git-mailinfo may corrupt patch headers on attached files

[Junio C Hamano <gitster@pobox.com>]

Don Zickus <dzickus@redhat.com> writes:

> @@ -814,6 +814,9 @@ static void handle_body(void)
>  				return;
>  		}
>  
> +		/* line may have changed after handling boundary, check len */
> +		len = strlen(line);
> +
>  		/* Unwrap transfer encoding */
>  		len = decode_transfer_encoding(line, sizeof(line), len);
>  		if (len < 0) {

Sorry, but I have to reject this.  The reason this function treats "len"
in an unnatural way is that you cannot do strlen(line) if you want to
handle patches that touch lines with embedded NUL in them.  Ideally, the
array line[] in the global scope should be replaced with a pair of "char
line[] and int linelen" (or strbuf) so that the code will always know how
long the line is, but the conversion done by cce8d6f (mailsplit and
mailinfo: gracefully handle NUL characters, 2008-05-16) and 9aa2309
(mailinfo: apply the same fix not to lose NULs in BASE64 and QP codepaths,
2008-05-25) were done in minimally invasive way, so not all codepath in
the program can deal with lines with embedded NULs.  For that reason, the
code still uses fgets() and strlen() everywhere, but the two patches
quoted above should be careful enough to allow NULs in the contents part
of the message (structural parts such as mime boundaries cannot have NUL
with the code, but it should not be a problem in practice).

The point you inserted strlen() above, however, is one of the places that
line[] has patch text and can have NUL in it, so strlen() there would
break the earlier fix.

Here is the minimum replacement patch, still not handling embedded NULs
anywhere in the structural part of the message, that should work.  Sane
MUAs should quote embedded NULs in the original contents with QP or BASE64
to protect them from handle_boundary() and other functions, and after
decoding, these embedded NULs will be kept by decode_transfer_encoding(),
so I think this would work Ok in practice.

I tested this with both Linus's test message and it does not break t5100.

---

 builtin-mailinfo.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 97c1ff9..fa6e8f9 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -812,6 +812,7 @@ static void handle_body(void)
 					      np - newline);
 			if (!handle_boundary())
 				return;
+			len = strlen(line);
 		}
 
 		/* Unwrap transfer encoding */