269390 - Files with mixed CR and LF attached as plain text results in file corruption

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Christian Franke]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041111
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041111

If a binary file contains mixed CR, LF and CRLF newlines and the remaining chars
are printable, it is attached as plain text.
All CR, LF and CRLF are converted to CRLF, received file will be corrupted.

Reproducible: Always
Steps to Reproduce:
1. Send file attached to comment 1 to yourself (do not store in Drafts folder)
2. Save the received attachment
3 [review]. Edit the received mail as new and send it to yourself
4. Save the received attachment
5. Compare the original and the two received files

Actual Results:  
All 3 Files differ.
(An extra newline has been added to the end of the file from step 4).


Expected Results:  
All 3 Files should be equal.


The current detection of newlines in nsMsgAttachmentHandler::AnalyzeDataChunk()
is too weak (see also bug 161806 comment 5).

An attachment should use Content-Transfer-Encoding: 7bit, 8bit or
quoted-printable only if:
- All newlines have the same flavor: CR, LF or CRLF.
- The last line ends with a newline, to prevent the difference between step 2
and 4. (This will also fix bug 237118)

Workaround: set "mail.file_attach_binary = true" or use a file extension that is
considered binary.

Related: Bug 161806, Bug 237118.

Comment 1

[Christian Franke]

Attachment: File with mixed CR LF CRLF (zipped to avoid conversion during up/download)

Comment 2

[Mike Cowperthwaite]

This problem is probably at the root of bug 195613 (same as bug 142517?) and bug 
176258

Comment 3

[Christian Franke]

Agree. Bug 195613 and bug 142517 are IMO duplicates of this bug (or vice versa
;-). A reworked plain text detection would also fix bug 176258 and bug 237118.

Comment 4

[Christian Franke]

Attachment: Patch for nsMsgAttachmentHandler::PickEncoding()

The patch forces base64 if CR LF CRLF are mixed or if the last line does not
end with a newline.

It includes also the patch from bug 237118 comment 4 (attachment 165256 [details] [diff] [review]).

Comment 5

[Christian Franke]

Attachment: Simplified version of the patch

Updating m_max_column in case of an incomplete last line is not necessary,
because m_current_column already contains this length.

This version of the patch does still fix bug 237118 and should also fix the
bugs from comment 2.

Comment 6

[David :Bienvenu]

Comment on attachment 200055 [details] [diff] [review]
Simplified version of the patch

thx for the patch. Some nits:

+      if (*s == nsCRT::CR) {

this brace should be on a newline, to be consistent.

(forceB64 || mime_type_requires_b64_p (m_type) ||
!(m_have_cr+m_have_lf+m_have_crlf == 1 && m_current_column == 0)))

this part would be more readable if it was all ||, so 

(forceB64 || mime_type_requires_b64_p (m_type) ||
m_have_cr+m_have_lf+m_have_crlf != 1 || m_current_column != 0))

this might be outside the scope of this patch, but does it make sense to keep
analyzing the file if we ever get a line that's terminated differently from all
the other lines?

An other possibility would have been to make m_have_cr, lf, crlf be PR_Bools
and use XOR to see that exactly one of them is set, ^, but that's up to you.

Comment 7

[Christian Franke]

Attachment: Patch with readability fixes from comment 6

> thx for the patch. Some nits:
> [...]
agree, done.

> this might be outside the scope of this patch, but does it make sense to keep

> analyzing the file if we ever get a line that's terminated differently from
all
> the other lines?
No, such a file should be encoded as binary.
The info about the actual encoding of newlines will (AFAIK) always be lost with
the encodings selected in the else case (7bit, 8bit or quoted-printable).

> An other possibility would have been to make m_have_cr, lf, crlf be PR_Bools
> and use XOR to see that exactly one of them is set, ^, but that's up to you.
XOR'ing would detect whether an odd number of them is set.
So this won't work if all 3 are set.

Using PRBools (which are actually ints instead of the new real "bool"s) would
also work for my patch.
I didn't use this to avoid violation any coding conventions which discourage
integer ops on PRBools.

BTW: Could you please request sr? from someone appropriate.

Comment 8

[David :Bienvenu]

Comment on attachment 200327 [details] [diff] [review]
Patch with readability fixes from comment 6

I'm happy to r/sr it, and check it in, if you don't have checkin rights.  Using
PRUint8 instead of PRBool is the right thing to do, since you're assigning ints
to them.

Comment 9

[Christian Franke]

(In reply to comment #8)
> I'm happy to r/sr it, and check it in, if you don't have checkin rights.
Yes, please. Unfortunately, I don't have checkin rights ;-)

Comment 10

[David :Bienvenu]

fix checked in, thx, Christian.

Comment 11

[Mike Cowperthwaite]

V with TB 1.6a1-1022, Win2K.

Comment 12

[Christian Franke]

Attachment: Fix for a regression introduced by my patch from comment 7

Sry, but (during editing a fix for Bug 194382) I found that my patch from comment 7 introduces the following regression:

A CRLF on a chunk boundary does not set m_have_crlf, but both m_have_cr and m_have_lf.
As a consequence, a regular text file with as least one CRLF on a boundary will be encoded as base64.
Due to the small chunk size of 256, this case is very likely.

This new patch fixes this and should still fix this bug and the related bugs.

Comment 13

[Christian Franke]

Reopened due to regression, see comment 12.

Comment 14

[David :Bienvenu]

Comment on attachment 200595 [details] [diff] [review]
Fix for a regression introduced by my patch from comment 7

can you call it m_prev_char_was_cr - it makes it slightly more readable.  While we're at it, we might as well increase the block size to 1K or so.

Comment 15

[Christian Franke]

Attachment: Fix changed according to comment 14

Comment 16

[David :Bienvenu]

second fix checked in, thx.

Comment 17

[Mike Cowperthwaite]

Bug 215005 may have been fixed by this bug's fix.

I'm not sure if the fixes made it into the 1.8.1 branch or not; if not, the approval requests for 1.8.0.2 should be extended to that branch as well.

Comment 18

[Scott MacGregor]

Comment on attachment 200327 [details] [diff] [review]
Patch with readability fixes from comment 6

approving for 1.8.1 if these changes aren't there already. 

Minusing for the security release.

Comment 19

[Mike Cowperthwaite]

I'm not sure how to use LXR to check if this made it onto 1.8.1 -- the branches presented by LXR have "1.8" and "1.8.0".  The "1.8" branch doesn't show this patch.  David, can you check this?

Comment 20

[Mike Cowperthwaite]

(In reply to comment #19)
> I'm not sure how to use LXR to check if this made it onto 1.8.1 -- the
> branches presented by LXR have "1.8" and "1.8.0".  The "1.8" branch doesn't
> show this patch.  David, can you check this?

I've confirmed that 2a1-0328 is not working correctly, while 3a1-0329 is.
This patch needs to be ported (already approved).  Requesting blocking so this doesn't get dropped.

Comment 21

[David :Bienvenu]

yes, Mike, I'll take care of it - I vaguely remember there being a few iterations on this, so I'll look at the CVS history.

Comment 22

[Scott MacGregor]

I've landed this on the 1.8.1 branch.