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

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Phil Lacy]

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

Comment 1

[WADA:World Anti-bad-Duping Agency]

> 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.

Comment 2

[David :Bienvenu]

thx, WADA, that's correct. "From " is sufficient.

Comment 3

[Phil Lacy]

Attachment: check for need of "From " mbox separator.

adds the from separator to external files copied into TB.

Comment 4

[Phil Lacy]

Attachment: fixed buffer mistake added check for filesize is complete or else abort copy.

Added buffer space.  Maybe we should note in nsIInputStream.idl, read() needs caller to allocate the buffer space.

Comment 5

[Phil Lacy]

Attachment: fixes braces on previous version of patch

Comment 6

[David :Bienvenu]

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...

Comment 7

[Phil Lacy]

Attachment: corrected filesize determination

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

Comment 8

[David :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.

Comment 9

[David :Bienvenu]

I'll land this, though I think I'm going to tweak the error handling a little bit first.

Comment 10

[David :Bienvenu]

fix checked in, thx, Phil.