'git am' breakage with MIME decoding

'git am' breakage with MIME decoding

[Linus Torvalds]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1979 bytes --]


Ok, so I generally try to avoid MIME-encoded emails because my old legacy 
tools didn't handle them, but since 'git am' is supposed to be able to 
handle them, I just tried one. And it failed.

Un-encoding them in the email client and then re-doing the thing worked 
fine, so it's definitely related to the MIME-decoding somehow.

I'm attaching both versions of the email so people can test it out (it 
applies to v2.6.26-rc9 of the kernel), but the behaviour in short is that 
the plain version (ie the one where I used my MUA to "export" the email 
without MIME crud) results in the correct:

	commit 97f8571e663c808ad2d01a396627235167291556
	Author: Philipp Zabel <philipp.zabel@gmail.com>
	Date:   Sun Jul 6 01:15:34 2008 +0200
	
	    pxamci: fix byte aligned DMA transfers
	    
	    The pxa27x DMA controller defaults to 64-bit alignment. This caused
	    the SCR reads to fail (and, depending on card type, error out) when
	    card->raw_scr was not aligned on a 8-byte boundary.
    ...

while the MIME-encoded version results in

	commit 92cdd47753abc9a6f1b8d96fedcbb5ed88b5ab57
	Author: Pierre Ossman <drzeus-list@drzeus.cx>
	Date:   Sun Jul 6 01:15:34 2008 +0200
	
	    pxamci: fix byte aligned DMA transfers
	    
	    F
	    The pxa27x DMA controller defaults to 64-bit alignment. This caused
	    the SCR reads to fail (and, depending on card type, error out) when
	    card->raw_scr was not aligned on a 8-byte boundary.
	    ...

ie notice how the "From: Philipp Zabel <philipp.zabel@gmail.com>" got 
corrupted somehow. It was apparently _partially_ recognized and removed, 
but it left the 'F' around, and probably because of the partial removal it 
then didn't get recognized as the author, so the original email sender 
(Pierre) got credit.

This is with a git version as of five minutes ago: v1.5.6.2-220-g44701c6.

Any ideas? I have not looked at it at all, since I'm not a fan of MIME, 
and didn't have anything to do with the MIME-decoding code.

			Linus

[-- Attachment #2: Type: TEXT/PLAIN, Size: 5661 bytes --]

From torvalds@linux-foundation.org Sat Jul  5 16:17:53 2008 -0700
Return-Path: <drzeus-list@drzeus.cx>
Received: from woody.linux-foundation.org (woody.linux-foundation.org [127.0.0.1])
	by woody.linux-foundation.org (8.14.2/8.14.2) with ESMTP id m65NHrMl003556
	for <torvalds@localhost>; Sat, 5 Jul 2008 16:17:53 -0700
Received: from imap1.linux-foundation.org [140.211.169.55]
	by woody.linux-foundation.org with IMAP (fetchmail-6.3.8)
	for <torvalds@localhost> (single-drop); Sat, 05 Jul 2008 16:17:53 -0700 (PDT)
Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13])
	by imap1.linux-foundation.org (8.13.5.20060308/8.13.5/Debian-3ubuntu1.1) with ESMTP id m65NGNAD010669
	for <torvalds@imap1.linux-foundation.org>; Sat, 5 Jul 2008 16:16:23 -0700
Received: from smtp.drzeus.cx (server.drzeus.cx [85.8.24.28])
	by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id m65NFhCI026377
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO)
	for <torvalds@linux-foundation.org>; Sat, 5 Jul 2008 16:15:46 -0700
Received: from mjolnir.drzeus.cx (wlan248.drzeus.cx [::ffff:10.8.2.248])
  (AUTH: LOGIN drzeus, TLS: TLSv1/SSLv3,256bits,AES256-SHA)
  by smtp.drzeus.cx with esmtp; Sun, 06 Jul 2008 01:15:39 +0200
  id 0000000000128003.000000004870009B.00003DBE
Date: Sun, 6 Jul 2008 01:15:34 +0200
From: Pierre Ossman <drzeus-list@drzeus.cx>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
        Philipp Zabel <philipp.zabel@gmail.com>,
        Stable branch <stable@kernel.org>
Subject: [PATCH] pxamci: fix byte aligned DMA transfers
Message-ID: <20080706011534.6dc71f5a@mjolnir.drzeus.cx>
X-Mailer: Claws Mail 3.4.0 (GTK+ 2.13.3; i386-redhat-linux-gnu)
Mime-Version: 1.0
Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=PGP-SHA1; boundary="=_freyr.drzeus.cx-15806-1215299739-0001-2"
Received-SPF: none (domain of drzeus-list@drzeus.cx does not designate permitted sender hosts)
X-Spam-Status: No, hits=-6.02 required=5 tests=AWL,BAYES_00,OSDL_HEADER_SUBJECT_BRACKETED,PATCH_SUBJECT_OSDL
X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__
X-MIMEDefang-Filter: lf$Revision: 1.188 $
X-Scanned-By: MIMEDefang 2.63 on 140.211.169.13
X-IMAPbase: 1215365788 1
Status: RO
X-Status: 
X-Keywords:                      
X-UID: 1

This is a MIME-formatted message.  If you see this text it means that your
E-mail software does not support MIME-formatted messages.

--=_freyr.drzeus.cx-15806-1215299739-0001-2
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

From: Philipp Zabel <philipp.zabel@gmail.com>

The pxa27x DMA controller defaults to 64-bit alignment. This caused
the SCR reads to fail (and, depending on card type, error out) when
card->raw_scr was not aligned on a 8-byte boundary.

For performance reasons all scatter-gather addresses passed to
pxamci_request should be aligned on 8-byte boundaries, but if
this can't be guaranteed, byte aligned DMA transfers in the
have to be enabled in the controller to get correct behaviour.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---
 drivers/mmc/host/pxamci.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 65210fc..d89475d 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -114,6 +114,7 @@ static void pxamci_setup_data(struct pxamci_host *host,=
 struct mmc_data *data)
 	unsigned int nob =3D data->blocks;
 	unsigned long long clks;
 	unsigned int timeout;
+	bool dalgn =3D 0;
 	u32 dcmd;
 	int i;
=20
@@ -152,6 +153,9 @@ static void pxamci_setup_data(struct pxamci_host *host,=
 struct mmc_data *data)
 		host->sg_cpu[i].dcmd =3D dcmd | length;
 		if (length & 31 && !(data->flags & MMC_DATA_READ))
 			host->sg_cpu[i].dcmd |=3D DCMD_ENDIRQEN;
+		/* Not aligned to 8-byte boundary? */
+		if (sg_dma_address(&data->sg[i]) & 0x7)
+			dalgn =3D 1;
 		if (data->flags & MMC_DATA_READ) {
 			host->sg_cpu[i].dsadr =3D host->res->start + MMC_RXFIFO;
 			host->sg_cpu[i].dtadr =3D sg_dma_address(&data->sg[i]);
@@ -165,6 +169,15 @@ static void pxamci_setup_data(struct pxamci_host *host=
, struct mmc_data *data)
 	host->sg_cpu[host->dma_len - 1].ddadr =3D DDADR_STOP;
 	wmb();
=20
+	/*
+	 * The PXA27x DMA controller encounters overhead when working with
+	 * unaligned (to 8-byte boundaries) data, so switch on byte alignment
+	 * mode only if we have unaligned data.
+	 */
+	if (dalgn)
+		DALGN |=3D (1 << host->dma);
+	else
+		DALGN &=3D (1 << host->dma);
 	DDADR(host->dma) =3D host->sg_dma;
 	DCSR(host->dma) =3D DCSR_RUN;
 }


--=20
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

--=_freyr.drzeus.cx-15806-1215299739-0001-2
Content-Type: application/pgp-signature; name="signature.asc"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename=signature.asc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)

iEYEARECAAYFAkhwAJsACgkQ7b8eESbyJLjDpwCgyde8Uz/u6iHD5/JwFyH6r8hA
dvQAoMH38ZvMg355D4R0jXmUXYfJzJds
=YuR1
-----END PGP SIGNATURE-----

--=_freyr.drzeus.cx-15806-1215299739-0001-2--


[-- Attachment #3: Type: TEXT/PLAIN, Size: 2964 bytes --]

From drzeus-list@drzeus.cx Sat Jul  5 16:17:53 2008
Date: Sun, 6 Jul 2008 01:15:34 +0200
From: Pierre Ossman <drzeus-list@drzeus.cx>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Philipp Zabel <philipp.zabel@gmail.com>, Stable branch <stable@kernel.org>
Subject: [PATCH] pxamci: fix byte aligned DMA transfers

From: Philipp Zabel <philipp.zabel@gmail.com>

The pxa27x DMA controller defaults to 64-bit alignment. This caused
the SCR reads to fail (and, depending on card type, error out) when
card->raw_scr was not aligned on a 8-byte boundary.

For performance reasons all scatter-gather addresses passed to
pxamci_request should be aligned on 8-byte boundaries, but if
this can't be guaranteed, byte aligned DMA transfers in the
have to be enabled in the controller to get correct behaviour.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---
 drivers/mmc/host/pxamci.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 65210fc..d89475d 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -114,6 +114,7 @@ static void pxamci_setup_data(struct pxamci_host *host, struct mmc_data *data)
 	unsigned int nob = data->blocks;
 	unsigned long long clks;
 	unsigned int timeout;
+	bool dalgn = 0;
 	u32 dcmd;
 	int i;
 
@@ -152,6 +153,9 @@ static void pxamci_setup_data(struct pxamci_host *host, struct mmc_data *data)
 		host->sg_cpu[i].dcmd = dcmd | length;
 		if (length & 31 && !(data->flags & MMC_DATA_READ))
 			host->sg_cpu[i].dcmd |= DCMD_ENDIRQEN;
+		/* Not aligned to 8-byte boundary? */
+		if (sg_dma_address(&data->sg[i]) & 0x7)
+			dalgn = 1;
 		if (data->flags & MMC_DATA_READ) {
 			host->sg_cpu[i].dsadr = host->res->start + MMC_RXFIFO;
 			host->sg_cpu[i].dtadr = sg_dma_address(&data->sg[i]);
@@ -165,6 +169,15 @@ static void pxamci_setup_data(struct pxamci_host *host, struct mmc_data *data)
 	host->sg_cpu[host->dma_len - 1].ddadr = DDADR_STOP;
 	wmb();
 
+	/*
+	 * The PXA27x DMA controller encounters overhead when working with
+	 * unaligned (to 8-byte boundaries) data, so switch on byte alignment
+	 * mode only if we have unaligned data.
+	 */
+	if (dalgn)
+		DALGN |= (1 << host->dma);
+	else
+		DALGN &= (1 << host->dma);
 	DDADR(host->dma) = host->sg_dma;
 	DCSR(host->dma) = DCSR_RUN;
 }


-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.


    [ Part 2, Application/PGP-SIGNATURE (Name: "signature.asc") 204 bytes. ]
    [ Unable to print this part. ]

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

[Don Zickus]

Boundary lines in emails are treated as a special case.  As a result of
processing the boundary line a new line will be read into the buffer.

The string length variable 'len' is evaluated before the boundary case, thus
there is the possibility the length of the string does not match the new
line read in (in the boundary line case).  This causes a partial output of
the line to the patch file.

The fix is trivial, evaluate the length of the string right before
processing it.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---

I noticed this the other day, just never got a chance to send the fix out.
This might be the same problem I ran into.

Cheers,
Don

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

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 2894e34..cedda18 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -795,7 +795,7 @@ static void handle_body(void)
 	int rc = 0;
 	static char newline[2000];
 	static char *np = newline;
-	int len = strlen(line);
+	int len;
 
 	/* Skip up to the first boundary */
 	if (content_top->boundary) {
@@ -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) {
-- 
1.5.6.rc2.48.g13da

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

[Linus Torvalds]

On Sun, 6 Jul 2008, Don Zickus wrote:
> 
> I noticed this the other day, just never got a chance to send the fix out.
> This might be the same problem I ran into.

Ack. This patch does indeed seem to fix the test-case I had. Thanks,

			Linus

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

[Junio C Hamano]

Don Zickus <dzickus@redhat.com> writes:

> Boundary lines in emails are treated as a special case.  As a result of
> processing the boundary line a new line will be read into the buffer.
>
> The string length variable 'len' is evaluated before the boundary case, thus
> there is the possibility the length of the string does not match the new
> line read in (in the boundary line case).  This causes a partial output of
> the line to the patch file.
>
> The fix is trivial, evaluate the length of the string right before
> processing it.

Ah, I was about to bisect this to see where it needs to be fixed and if it
needs to be fixed in maint (or maint-1.5.5 and earlier).  Thanks for doing
this before I got around to it.

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

[Junio C Hamano]

> I noticed this the other day, just never got a chance to send the fix out.
> This might be the same problem I ran into.
>
> Cheers,
> Don
>
>  builtin-mailinfo.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 2894e34..cedda18 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -795,7 +795,7 @@ static void handle_body(void)
>  	int rc = 0;
>  	static char newline[2000];
>  	static char *np = newline;
> -	int len = strlen(line);
> +	int len;
>  
>  	/* Skip up to the first boundary */
>  	if (content_top->boundary) {
> @@ -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) {

This does fix the "F\n" issue, but seems to break t5100 test ("respect
NULs").  I haven't looked into the details yet...

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

[Junio C Hamano]

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 */

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

[Don Zickus]

On Sun, Jul 06, 2008 at 10:19:46PM -0700, Junio C Hamano wrote:
> 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.

Good thing for test cases. :-)  Thanks for the explanation.

ACK

Cheers,
Don