Closed
Bug 269390
Opened 20 years ago
Closed 19 years ago
Files with mixed CR and LF attached as plain text results in file corruption
Categories
(MailNews Core :: Attachments, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Franke, Assigned: Franke)
References
Details
(Keywords: fixed1.8.1, verified1.8.1.3)
Attachments
(3 files, 3 obsolete files)
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
mscott
:
approval-branch-1.8.1+
mscott
:
approval1.8.0.2-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Bienvenu
:
review+
mscott
:
approval-branch-1.8.1+
mscott
:
approval1.8.0.2-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•20 years ago
|
||
Updated•20 years ago
|
Product: MailNews → Core
Comment 2•19 years ago
|
||
This problem is probably at the root of bug 195613 (same as bug 142517?) and bug
176258
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 3•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
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]).
Attachment #199922 -
Flags: review?(bienvenu)
Assignee | ||
Comment 5•19 years ago
|
||
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.
Attachment #199922 -
Attachment is obsolete: true
Attachment #200055 -
Flags: review?(bienvenu)
Assignee | ||
Updated•19 years ago
|
Attachment #199922 -
Flags: review?(bienvenu)
Comment 6•19 years ago
|
||
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.
Attachment #200055 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 7•19 years ago
|
||
> 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.
Attachment #200055 -
Attachment is obsolete: true
Attachment #200327 -
Flags: review?(bienvenu)
Comment 8•19 years ago
|
||
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.
Attachment #200327 -
Flags: superreview+
Attachment #200327 -
Flags: review?(bienvenu)
Attachment #200327 -
Flags: review+
Assignee | ||
Comment 9•19 years ago
|
||
(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•19 years ago
|
||
fix checked in, thx, Christian.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•19 years ago
|
||
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.
Attachment #200595 -
Flags: review?(bienvenu)
Assignee | ||
Comment 13•19 years ago
|
||
Reopened due to regression, see comment 12.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 14•19 years ago
|
||
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.
Assignee | ||
Comment 15•19 years ago
|
||
Attachment #200595 -
Attachment is obsolete: true
Attachment #200618 -
Flags: review?(bienvenu)
Attachment #200595 -
Flags: review?(bienvenu)
Updated•19 years ago
|
Attachment #200618 -
Flags: review?(bienvenu) → review+
Updated•19 years ago
|
Assignee: sspitzer → Franke
Status: REOPENED → NEW
Comment 16•19 years ago
|
||
second fix checked in, thx.
Status: NEW → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #200327 -
Flags: approval1.8.0.2?
Updated•19 years ago
|
Attachment #200618 -
Flags: approval1.8.0.2?
Comment 17•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #200327 -
Flags: approval1.8.0.2?
Attachment #200327 -
Flags: approval1.8.0.2-
Attachment #200327 -
Flags: approval-branch-1.8.1+
Updated•19 years ago
|
Attachment #200618 -
Flags: approval1.8.0.2?
Attachment #200618 -
Flags: approval1.8.0.2-
Attachment #200618 -
Flags: approval-branch-1.8.1+
Comment 19•19 years ago
|
||
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•19 years ago
|
||
(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.
Flags: blocking-thunderbird2?
Comment 21•19 years ago
|
||
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.
Updated•18 years ago
|
Flags: blocking-thunderbird2? → blocking-thunderbird2+
Updated•18 years ago
|
Keywords: verified1.8.1.3
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•