>It certainly seems like it won't work properly on servers using maildir storage (they don't have "From " lines between messages, although messages _could_ still begin with a "From " sent down from an errant IMAP server, say). The whole function just seems a bit problematic. >::: mailnews/base/src/nsMsgDBFolder.cpp ``` @@ +696,4 @@ > // check if message starts with From, or is a draft and starts with FCC > + if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) || > + ((mFlags & nsMsgFolderFlags::Drafts) && > + !strncmp(startOfMsg, "FCC", 3)))) { ``` >This seems bad (it was bad in the original code) - if we're using maildir, there should never be a "From " line (not quite sure about FCC). In that case, it'll skip this whole block and return with NS_OK, without skipping the X_MOZILLA_STATUS lines. I don't run with maildir but I have tried it in the past and have some archived maildir files produced by tb and they do have the same lines as mbox, e.g., taken from a file called .../cur/1545970334263000 : ``` From - Thu Dec 27 23:12:14 2018 X-Mozilla-Status: 0001 X-Mozilla-Status2: 00000000 Return-Path: <orealius@juno.com> Received: from impin007 ([68.114.189.31]) : ``` The original code would have rejected this if it didn't start with the "From ". In my new t3.diff I now fail and mark offline if the initial "From " is missing. In the previous diff, a missing leading "From " was accepted as you pointed out. But if maildir file format has changed and no longer has the leading "From -" then the problem you point out does exist. >@@ +696,5 @@ ``` > // check if message starts with From, or is a draft and starts with FCC > + if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) || > + ((mFlags & nsMsgFolderFlags::Drafts) && > + !strncmp(startOfMsg, "FCC", 3)))) { > + startOfMsg[bytesRead] = '\0'; ``` > I think this needs to be moved up before the strncmps - otherwise the strncmps have the potential of reading into uninitialised bytes in startOfMsg. Good point! I put it back where it was. Not sure what I was thinking by moving that line. ``` > + // No colon or "From" but containes EOL marker. Go ahead and tolerate ``` >contains Fixed. I was thinking this function is only used with mbox, but I guess not. There are several callers of this so having each place deal with the return code and marking as offline sounds like a pretty big change. From my reading of the original rationale for this, the theory was that the file was somehow corrupted but the server content was OK. So if an mbox file record got written again with the server content it would be "good" after that (leaving a single orphan record). But it seems that we mostly just see bad headers in the server which cause the mbox file go "grow" due to being marked offline causing lot of orphan records. Not sure if this applies ot maildir, i.e., would you end up with lots of duplicate orphan maildir files?
Bug 1702692 Comment 33 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
>It certainly seems like it won't work properly on servers using maildir storage (they don't have "From " lines between messages, although messages _could_ still begin with a "From " sent down from an errant IMAP server, say). The whole function just seems a bit problematic. >::: mailnews/base/src/nsMsgDBFolder.cpp ``` @@ +696,4 @@ > // check if message starts with From, or is a draft and starts with FCC > + if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) || > + ((mFlags & nsMsgFolderFlags::Drafts) && > + !strncmp(startOfMsg, "FCC", 3)))) { ``` >This seems bad (it was bad in the original code) - if we're using maildir, there should never be a "From " line (not quite sure about FCC). In that case, it'll skip this whole block and return with NS_OK, without skipping the X_MOZILLA_STATUS lines. I don't run with maildir but I have tried it in the past and have some archived maildir files produced by tb and they do have the same lines as mbox, e.g., taken from a file called .../cur/1545970334263000 : ``` From - Thu Dec 27 23:12:14 2018 X-Mozilla-Status: 0001 X-Mozilla-Status2: 00000000 Return-Path: <orealius@juno.com> Received: from impin007 ([68.114.189.31]) : ``` The original code would have rejected this if it didn't start with the "From ". In my new t3.diff I now fail and mark offline if the initial "From " is missing. In the previous diff, a missing leading "From " was accepted as you pointed out. But if maildir file format has changed and no longer has the leading "From -" then the problem you point out does exist. >@@ +696,5 @@ ``` > // check if message starts with From, or is a draft and starts with FCC > + if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) || > + ((mFlags & nsMsgFolderFlags::Drafts) && > + !strncmp(startOfMsg, "FCC", 3)))) { > + startOfMsg[bytesRead] = '\0'; ``` > I think this needs to be moved up before the strncmps - otherwise the strncmps have the potential of reading into uninitialised bytes in startOfMsg. Good point! I put it back where it was. Not sure what I was thinking by moving that line. ``` > + // No colon or "From" but containes EOL marker. Go ahead and tolerate ``` >contains Fixed. I was thinking this function is only used with mbox, but I guess not. There are several callers of this so having each place deal with the return code and marking as offline sounds like a pretty big change. From my reading of the original rationale for this, the theory was that the file was somehow corrupted but the server content was OK. So if an mbox file record got written again with the server content it would be "good" after that (leaving a single orphan record). But it seems that we mostly just see bad headers in the server which cause the mbox file go "grow" due to being marked offline causing lot of orphan records. Not sure if this applies ot maildir, i.e., would you end up with lots of duplicate orphan maildir files? Note: Repair or compact doesn't get rid of the orphan mbox records. You have to manually delete the mbox and msf files and let them re-download (with the bad header hopefully fixed in the server or you end up back where you were).