Closed Bug 499304 Opened 13 years ago Closed 13 years ago

localfolders CopyFileMessage doesn't check for mozilla envelope lines and may copy partial email files

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

(1 file, 3 obsolete files)

need to add in check for 'From -" line and mozilla status lines.  Otherwise copying non-mozilla eml will break local store.
> need to add in check for 'From -" line  (snip)

AFAIK, it should be "From " line instead of "From -" line.
"-" part by Tb can be any non-null word(Opera 6 used null-word for it in his unix mbox style file though...). Tb simply uses "-" for it when Tb adds mail data to local store(unox mbox file).
If "From -" instead of "From " is checked/escaped, I guess Tb's Rebuild-Index and other software who reads Tb's unix mbox file are affected by "From " line or "From  ..." line.
thx, WADA, that's correct. "From " is sufficient.
adds the from separator to external files copied into TB.
Attachment #384261 - Flags: review?(bienvenu)
Attachment #384261 - Flags: review?(bienvenu)
Added buffer space.  Maybe we should note in nsIInputStream.idl, read() needs caller to allocate the buffer space.
Attachment #384261 - Attachment is obsolete: true
Blocks: 498978
Attachment #384309 - Attachment description: fixed → fixed buffer mistake added check for filesize is complete or else abort copy.
Attachment #384309 - Flags: review?(bienvenu)
Summary: localfolders CopyFileMessage doesn't check for mozilla envelope lines → localfolders CopyFileMessage doesn't check for mozilla envelope lines and may copy partial email files
Attachment #384309 - Attachment is obsolete: true
Attachment #385499 - Flags: review?(bienvenu)
Attachment #384309 - Flags: review?(bienvenu)
Flags: blocking-thunderbird3+
Keywords: dataloss
Comment on attachment 385499 [details] [diff] [review]
fixes braces on previous version of patch

+      aFile->GetFileSize(&diskFileSize);
+      rv = diskFileSize != fileSize;

rv is an nsresult, not a boolean, so you should do something like

if (diskFileSize != fileSize)
  rv = some ns error code - I'm not sure what error code would be appropriate, since I don't know why this condition would arise...

+        if (strncmp(buffer, "From ", 5) != 0)

you can lose the != 0 part.

Other than that, this looks OK, other than that it would be nice to add a comment explaining what you're doing...
the nsIInputStream available() spec indicates that it does not indicate all data that can be read from stream.

Let me know if we need a better error code for max message file being exceeded. I don't see CopyData()  intending to set the max message size at PRInt32.

Can we change CopyData length to PRInt64
Attachment #385499 - Attachment is obsolete: true
Attachment #387808 - Flags: review?(bienvenu)
Attachment #385499 - Flags: review?(bienvenu)
Whiteboard: [needs review bienvenu]
Comment on attachment 387808 [details] [diff] [review]
corrected filesize determination

I'm not sure if a > 2GB message is an actual possibility, but the way to fix that would be to call CopyData in a loop.
Attachment #387808 - Flags: superreview+
Attachment #387808 - Flags: review?(bienvenu)
Attachment #387808 - Flags: review+
I'll land this, though I think I'm going to tweak the error handling a little bit first.
fix checked in, thx, Phil.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs review bienvenu]
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → Thunderbird 3.0b4
You need to log in before you can comment on or make changes to this bug.