Closed Bug 498978 Opened 15 years ago Closed 15 years ago

CopyFileMessage truncates messages with no ending cr/lf

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: philbaseless-firefox, Assigned: philbaseless-firefox)

References

Details

(Keywords: dataloss)

Attachments

(2 files, 4 obsolete files)

copy an email message file that doesn't have cr/lf and it will give an error and not copy over last line of message.
cause of problem is here:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2382
error is from left over buffer reported here:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2516
doesn't happen when downloading IMAP messages, but a cr/lf is added to downloaded imap messages.

Existing unit test may prove this, I'll check later if not addressed in meantime.
Assignee: nobody → philbaseless-firefox
Depends on: 499304
This takes away the value of saving a partial line, and adding to new data on the next call.  The CopyFileMessage doesn't do this so it needs CopyData to do the whole thing, crlf at eof or not.
I need to pass through a variable for those calls that don't want the wholeFile functionality but to save partial lines.
I left it alone for now to see if you have a better idea. So far I think there is only one other call to CopyData.
Attachment #384326 - Flags: review?(bienvenu)
Attachment #384326 - Flags: review?(bienvenu)
Comment on attachment 384326 [details] [diff] [review]
copies over the last line that is 'leftover'

mbox format requires a blank line at end of message so I'll fix this and resubmit patch to add a blank line to files as needed.  This will make the CopyData work for all callers.
This takes care of CopyFileMessage to account for files that do not have a crlf at the eof.
But I'm not sure if the users of CopyData need this functionality. David, please check and see if we want to set this boolean value on the end of other CopyData uses.  Otherwise the init is setting it to false.
Attachment #384326 - Attachment is obsolete: true
Attachment #384797 - Flags: review?(bienvenu)
Comment on attachment 384797 [details] [diff] [review]
adds test for complete file copy and checks for crlf

This file doesn't use K&R braces, so this patch shouldn't...

m_completeFile is an ambiguous var name - I assume the sense of it is whether we should a trailing blank line? Could you use a name that reflects that?

On Mac/Linux, don't we just want a LF, not a CRLF?
Attachment #384797 - Flags: review?(bienvenu) → review-
Attached patch per review comments (obsolete) — Splinter Review
variable suggestion makes much better sense. Other comments fixed.
Attachment #384797 - Attachment is obsolete: true
Attachment #385498 - Flags: review?(bienvenu)
Comment on attachment 385498 [details] [diff] [review]
per review comments

this patch doesn't apply cleanly, but more seriously, I think it adds a blank line at the end of every block, not just at the end of the message.
(In reply to comment #6)
> (From update of attachment 385498 [details] [diff] [review])
> this patch doesn't apply cleanly, but more seriously, I think it adds a blank
> line at the end of every block, not just at the end of the message.

I had this depending on bug 499304. Both bugs are important to CopyFileMessage. But I think I can change the patch order if needed.

CopyFileMessage sets the m_addNewLineToEndOfMsg and calls CopyData with length of complete file (that was original intent of using m_completeFile as name, although I did change it to that from my first idea of m_wholeFile).

The 'while (1)' block has to copy the whole file, a line at a time, and if the last line of data is not terminated with a linebreak it adds it.
Flags: blocking-thunderbird3?
Keywords: dataloss
Flags: blocking-thunderbird3? → blocking-thunderbird3+
The CopyData method is called from an other place, in a loop, so it's a bit scary to add code that could add newlines in the middle of a message. I can see why you did it there, because you want to make sure the added CRLF goes through the normal line parsing stuff...

What might make this a lot cleaner is a comment explaining what the situation is, and what the code is doing. Otherwise, it just looks like magic.

Something like this, where you handle the case of not finding a cr or lf:

In the case of copying a whole file (i.e., called from CopyFileMessage), if we don't find a trailing LINEBREAK, we must be at the last line of the file, and it must not have a LINEBREAK, so we're going to add it to the buffer.
I quasi adapted your comment suggestion but feel free to edit it and the variable name if it's still not clear enough.
Attachment #385498 - Attachment is obsolete: true
Attachment #387813 - Flags: review?(bienvenu)
Attachment #385498 - Flags: review?(bienvenu)
Attachment #387813 - Attachment is obsolete: true
Attachment #387814 - Flags: review?(bienvenu)
Attachment #387813 - Flags: review?(bienvenu)
ok, thx, yes, I'm going to edit this a little to make it clear that the whole message is being copied in a single call to CopyData - that's really the crucial point.  I'll tweak this when the tree opens again, and land it; thx for the patch! I'll mark it r/sr=me when I attach the tweaked version later.
(In reply to comment #11)
> crucial point.  I'll tweak this when the tree opens again, and land it; thx for

I don't know if you applied bug 499304 but remember that needs to go in first or else you need to have me fix this patch to build on trunk.
just to save time when you are ready to land this.  I did a diff off the trunk as of today.
Whiteboard: [needs bienvenu to move forward]
Status: NEW → ASSIGNED
Comment on attachment 389324 [details] [diff] [review]
patch diff to trunk

I tweaked the variable name and added some comments to make it clearer what's going on, and checked in. Thx for the patch.
Attachment #389324 - Flags: superreview+
Attachment #389324 - Flags: review+
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs bienvenu to move forward]
Comment on attachment 387814 [details] [diff] [review]
header file change see other version for comments

clearing request
Attachment #387814 - Flags: review?(bienvenu)
Target Milestone: --- → Thunderbird 3.0b3
oups
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
OS: Windows XP → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: