Bug 1931258 - Make sure reading messages from beyond the end of an mbox file throws a read error. r=darktrojan,kaie

nsFileInputStream will happily let you Seek() past the end, and Read() from there, all without causing an error.
The Read() just returns 0 bytes of data (indicating EOF).

So this patch just tweaks the mbox parser to _require_ that there is a
message at the given position. Previously there was an exception to allow
nsMsgBrkMBoxStore::AsyncScan() to handle the corner case of an empty mbox
file. But AsyncScan() works fine with this patch, so we're OK.

Differential Revision: https://phabricator.services.mozilla.com/D228941

--HG--
extra : amend_source : d25b97a384718eb6f69aa7b2728ed339f1a3c10c

mailnews/base/src/MboxMsgInputStream.cpp

@@ -176,7 +176,8 @@ class MboxParser {
     if (mState == eMessageComplete) {
       mEnvAddr.Truncate();
       mEnvDate = 0;
-      mState = eExpectFromLine;
+      // This time around an EOF would also be acceptable.
+      mState = eExpectFromLineOrEOF;
     }
   }
 
@@ -222,7 +223,8 @@ class MboxParser {
     eEmitBodyLine,
     eMessageComplete,  // Message is complete (or ended prematurely).
     eMalformed,        // Error. No initial "From " line was found.
-    eEOF,              // End of mbox.
+    eExpectFromLineOrEOF,
+    eEOF,  // End of mbox.
   } mState{eExpectFromLine};
 
   // handle_<state>() functions consume as much data as they need, and
@@ -231,11 +233,19 @@ class MboxParser {
   // the mbox file has been reached.
   span handle(span data) {
     {
-      const char* stateName[] = {
-          "eExpectFromLine",  "eDiscardFromLine", "eExpectHeaderLine",
-          "eEmitHeaderLine",  "eEmitSeparator",   "eExpectBodyLine",
-          "eCountQuoting",    "eEmitQuoting",     "eEmitBodyLine",
-          "eMessageComplete", "eMalformed",       "eEOF"};
+      const char* stateName[] = {"eExpectFromLine",
+                                 "eDiscardFromLine",
+                                 "eExpectHeaderLine",
+                                 "eEmitHeaderLine",
+                                 "eEmitSeparator",
+                                 "eExpectBodyLine",
+                                 "eCountQuoting",
+                                 "eEmitQuoting",
+                                 "eEmitBodyLine",
+                                 "eMessageComplete",
+                                 "eMalformed",
+                                 "eExpectFromLineOrEOF",
+                                 "eEOF"};
       MOZ_LOG(gMboxLog, LogLevel::Verbose,
               ("MboxParser - handle %s (%zu bytes: '%s')", stateName[mState],
                data.Length(),
@@ -264,6 +274,8 @@ class MboxParser {
         return handle_eMessageComplete(data);
       case eMalformed:
         return handle_eMalformed(data);
+      case eExpectFromLineOrEOF:
+        return handle_eExpectFromLineOrEOF(data);
       case eEOF:
         return handle_eEOF(data);
       default:
@@ -323,12 +335,14 @@ class MboxParser {
     envDate = tmpDate;
   }
 
-  // We're expecting a new message to start, or an EOF.
+  // We expect a message. If we dont find one, it's a malformed mbox.
+  // NOTE: It turns out that if you Seek() way past the end of a file,
+  // performing reads will just return an EOF, rather than an error.
+  // If a storeToken is corrupted and we're actually positioned out
+  // past the end of the mbox file, the resulting EOF will safely cause
+  // us to be kicked out into eMalformed state which will correctly return
+  // an error.
   span handle_eExpectFromLine(span data) {
-    if (data.Length() < 5) {  // Enough to check for "From "?
-      mState = eEOF;          // no more messages.
-      return span();          // discard data
-    }
     if (IsFromLine(data)) {
       // The "From " line could have an email address (up to 254 bytes) and a
       // date string (24 bytes). MinChunk is tuned to avoid spliting up long
@@ -516,12 +530,12 @@ class MboxParser {
     return data;
   }
 
-  // All done, so this is a no-op.
+  // All done, so this is a no-op - just kick us into next state.
   span handle_eMessageComplete(span data) {
     if (data.IsEmpty()) {
       mState = eEOF;
     } else {
-      mState = eExpectFromLine;
+      mState = eExpectFromLineOrEOF;
     }
     return data;
   }
@@ -529,6 +543,19 @@ class MboxParser {
   // Halt parsing, So this is a no-op.
   span handle_eMalformed(span data) { return data; }
 
+  // We've finished a message and been Kick()ed back into life, so now expect
+  // another message or an EOF.
+  span handle_eExpectFromLineOrEOF(span data) {
+    if (data.Length() == 0) {
+      // All done. No more messages.
+      mState = eEOF;
+    } else {
+      // Not yet EOF, so we expect next message.
+      mState = eExpectFromLine;
+    }
+    return data;
+  }
+
   // All done, so this is a no-op.
   span handle_eEOF(span data) {
     MOZ_ASSERT(data.IsEmpty());
@@ -542,7 +569,7 @@ class MboxParser {
     // We don't go directly to eEOF.
     // Going to eMessageComplete holds parsing up until the output
     // has all been drained.
-    // After this, eExpectFromLine will move us into eEOF.
+    // After this, eExpectFromLineOrEOF will move us into eEOF.
     mState = eMessageComplete;
     Emit(data);
     return data.Last<0>();

mailnews/base/src/MboxMsgInputStream.h

@@ -46,7 +46,8 @@ class MboxMsgInputStream : public nsIInputStream {
    * given number. This is useful if the mbox stream doesn't contain the
    * expected sepatarors.
    */
-  explicit MboxMsgInputStream(nsIInputStream* mboxStream, uint32_t maxAllowedSize);
+  explicit MboxMsgInputStream(nsIInputStream* mboxStream,
+                              uint32_t maxAllowedSize);
 
   MboxMsgInputStream() = delete;
 
@@ -72,9 +73,11 @@ class MboxMsgInputStream : public nsIInputStream {
   nsresult Continue(bool& more);
 
   /**
-   * Return the offset into the underlying raw mbox stream at which the current
-   * message is located. This would be the location of the "From " separator
-   * line.
+   * Return the offset at which the current message is located. This would be
+   * the location of the "From " separator line.
+   * NOTE: this value is relative to where the underlying stream was
+   * positioned when the MboxMsgInputStream was constructed!
+   * If a seek was performed beforehand, that position is considered offset 0.
    */
   uint64_t MsgOffset() { return mMsgOffset; }
 

mailnews/local/src/nsMsgBrkMBoxStore.cpp

@@ -1418,7 +1418,12 @@ nsMsgBrkMBoxStore::GetMsgInputStream(nsIMsgFolder* aMsgFolder,
   nsCOMPtr<nsISeekableStream> seekable(do_QueryInterface(rawMboxStream));
   rv = seekable->Seek(PR_SEEK_SET, offset);
   NS_ENSURE_SUCCESS(rv, rv);
-  // Stream to return a single message, hiding all "From "-separator guff.
+  // Build stream to return a single message from the msgStore.
+  // NOTE: It turns out that Seek()ing way past the end of the file doesn't
+  // cause an error. And reading from there doesn't return an error either
+  // (just an EOF).
+  // But it's OK - MboxMsgInputStream will handle that case, and its Read()
+  // method will safely return an error (NS_MSG_ERROR_MBOX_MALFORMED).
   RefPtr<MboxMsgInputStream> msgStream =
       new MboxMsgInputStream(rawMboxStream, aMaxAllowedSize);
   msgStream.forget(aResult);

mailnews/local/test/unit/test_mboxMalformed.js

@@ -79,10 +79,16 @@ add_task(async function test_badStoreTokens() {
     generator.makeMessages({ count: 10 }).map(m => m.toMessageString())
   );
 
-  // Corrupt the storeTokens by adding 3.
+  // Corrupt the storeTokens in a couple of different ways.
+  let even = true;
   for (const msg of inbox.messages) {
-    const offset = Number(msg.storeToken) + 3;
-    msg.storeToken = offset.toString();
+    if (even) {
+      const offset = Number(msg.storeToken) + 3;
+      msg.storeToken = offset.toString();
+    } else {
+      msg.storeToken = "12345678"; // Past end of mbox file.
+    }
+    even = !even;
   }
 
   // Check that message reads fail.