diff --git a/mailnews/base/public/msgCore.h b/mailnews/base/public/msgCore.h index 3194e862f9..c16b37e98f 100644 --- a/mailnews/base/public/msgCore.h +++ b/mailnews/base/public/msgCore.h @@ -171,6 +171,9 @@ class nsIMsgFolder; #define NS_MSG_ERROR_UNEXPECTED_SIZE NS_MSG_GENERATE_FAILURE(35) +// Mbox message doesn't start with "From " separator line. +#define NS_MSG_ERROR_MBOX_MALFORMED NS_MSG_GENERATE_FAILURE(36) + /* Error codes for message compose are defined in compose\src\nsMsgComposeStringBundle.h. Message compose use the same error code space as other mailnews modules. To avoid any conflict, values between diff --git a/mailnews/base/src/MboxMsgInputStream.cpp b/mailnews/base/src/MboxMsgInputStream.cpp index b796ef99ec..c012169a9f 100644 --- a/mailnews/base/src/MboxMsgInputStream.cpp +++ b/mailnews/base/src/MboxMsgInputStream.cpp @@ -62,9 +62,16 @@ class MboxParser { * via Drain(). */ bool IsFinished() const { - return Available() == 0 && (mState == eEOF || mState == eMessageComplete); + return Available() == 0 && (mState == eEOF || mState == eMessageComplete || + mState == eMalformed); } + /** + * Returns true if the parser has decided the mbox is malformed and refuses + * to proceed (e.g. Missing a "From " line at the beginning). + */ + bool IsMalformed() const { return mState == eMalformed; } + /** * Returns true when the end of the mbox has been reached (and the last * message has been completely read out via Drain()). @@ -113,7 +120,8 @@ class MboxParser { while (true) { // If a message is complete (or the mbox is finished), then // we stall. - if (mState == eMessageComplete || mState == eEOF) { + if (mState == eMessageComplete || mState == eEOF || + mState == eMalformed) { break; } @@ -212,6 +220,7 @@ class MboxParser { eEmitQuoting, eEmitBodyLine, eMessageComplete, // Message is complete (or ended prematurely). + eMalformed, // Error. No initial "From " line was found. eEOF, // End of mbox. } mState{eExpectFromLine}; @@ -221,17 +230,11 @@ 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", - "eEOF"}; + const char* stateName[] = { + "eExpectFromLine", "eDiscardFromLine", "eExpectHeaderLine", + "eEmitHeaderLine", "eEmitSeparator", "eExpectBodyLine", + "eCountQuoting", "eEmitQuoting", "eEmitBodyLine", + "eMessageComplete", "eMalformed", "eEOF"}; MOZ_LOG(gMboxLog, LogLevel::Verbose, ("MboxParser - handle %s (%zu bytes: '%s')", stateName[mState], data.Length(), @@ -258,6 +261,8 @@ class MboxParser { return handle_eEmitBodyLine(data); case eMessageComplete: return handle_eMessageComplete(data); + case eMalformed: + return handle_eMalformed(data); case eEOF: return handle_eEOF(data); default: @@ -339,10 +344,9 @@ class MboxParser { } mState = eDiscardFromLine; } else { - MOZ_LOG(gMboxLog, LogLevel::Warning, + MOZ_LOG(gMboxLog, LogLevel::Error, ("MboxParser - Missing 'From ' separator")); - // Just jump straight to header phase. - mState = eExpectHeaderLine; + mState = eMalformed; } return data; } @@ -521,6 +525,9 @@ class MboxParser { return data; } + // Halt parsing, So this is a no-op. + span handle_eMalformed(span data) { return data; } + // All done, so this is a no-op. span handle_eEOF(span data) { MOZ_ASSERT(data.IsEmpty()); @@ -871,6 +878,10 @@ nsresult MboxMsgInputStream::PumpData() { mUnused -= consumed; } + if (mParser->IsMalformed()) { + return NS_MSG_ERROR_MBOX_MALFORMED; + } + return NS_OK; } diff --git a/mailnews/base/test/gtest/Helpers.cpp b/mailnews/base/test/gtest/Helpers.cpp index fe2343866c..086bca4ada 100644 --- a/mailnews/base/test/gtest/Helpers.cpp +++ b/mailnews/base/test/gtest/Helpers.cpp @@ -12,13 +12,16 @@ namespace testing { -void ExtractFromMbox(nsACString const& mbox, nsTArray& msgs, - size_t readSize) { +// Parses all the messages in mbox returning them as an array. +nsresult ExtractFromMbox(nsACString const& mbox, nsTArray& msgs, + size_t readSize) { // Open stream for raw mbox. nsCOMPtr raw; nsresult rv = NS_NewByteInputStream(getter_AddRefs(raw), mozilla::Span(mbox), NS_ASSIGNMENT_COPY); - ASSERT_TRUE(NS_SUCCEEDED(rv)); + if (NS_FAILED(rv)) { + return rv; + } msgs.Clear(); // Wrap with MboxMsgInputStream and read single message. @@ -27,7 +30,9 @@ void ExtractFromMbox(nsACString const& mbox, nsTArray& msgs, while (true) { nsAutoCString got; rv = Slurp(rdr, readSize, got); - ASSERT_TRUE(NS_SUCCEEDED(rv)); + if (NS_FAILED(rv)) { + return rv; + } // Corner case: suppress dud message for empty mbox file. if (!rdr->IsNullMessage()) { msgs.AppendElement(got); @@ -35,11 +40,14 @@ void ExtractFromMbox(nsACString const& mbox, nsTArray& msgs, // Try and reuse the MboxMsgInputStream for the next message. bool more; rv = rdr->Continue(more); - ASSERT_TRUE(NS_SUCCEEDED(rv)); + if (NS_FAILED(rv)) { + return rv; + } if (!more) { break; } } + return NS_OK; } // Read all the data out of a stream into a string, reading readSize @@ -50,7 +58,9 @@ nsresult Slurp(nsIInputStream* src, size_t readSize, nsACString& out) { while (true) { uint32_t n; nsresult rv = src->Read(readbuf.Elements(), readbuf.Length(), &n); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_FAILED(rv)) { + return rv; + } if (n == 0) { break; // EOF. } diff --git a/mailnews/base/test/gtest/Helpers.h b/mailnews/base/test/gtest/Helpers.h index 1b973eff09..24995ddf52 100644 --- a/mailnews/base/test/gtest/Helpers.h +++ b/mailnews/base/test/gtest/Helpers.h @@ -24,8 +24,8 @@ namespace testing { * produce bad results at different read sizes, so this can be used to * shake it out by asking for, say, a single byte at a time. */ -void ExtractFromMbox(nsACString const& mbox, nsTArray& msgs, - size_t readSize = 4096); +nsresult ExtractFromMbox(nsACString const& mbox, nsTArray& msgs, + size_t readSize = 4096); /** * Slurp just reads the src stream until EOF, returning the data in diff --git a/mailnews/base/test/gtest/TestMsgMboxRead.cpp b/mailnews/base/test/gtest/TestMsgMboxRead.cpp index c55704d509..e4f1f671e4 100644 --- a/mailnews/base/test/gtest/TestMsgMboxRead.cpp +++ b/mailnews/base/test/gtest/TestMsgMboxRead.cpp @@ -41,7 +41,8 @@ static void dumpCase(testing::MboxCase const& t, // readSize lets the caller set the size of the stream Read() calls. static void runInputStreamCase(testing::MboxCase const& t, size_t readSize) { nsTArray msgs; - testing::ExtractFromMbox(t.mbox, msgs, readSize); + nsresult rv = testing::ExtractFromMbox(t.mbox, msgs, readSize); + ASSERT_TRUE(NS_SUCCEEDED(rv)); // Display test data + results before asserting to make troubleshooting // simpler. @@ -154,3 +155,17 @@ TEST(TestMsgMboxRead, Ambiguities) } } } + +// Test to handle missing From line. +// Not too exhaustive (more in xpcshell tests). +// Just make sure that a missing "From " line causes an error. +TEST(TestMsgMboxRead, MissingFromLine) +{ + nsAutoCString borkedMbox("blah blah blah"); + + for (size_t s : {1, 3, 4096}) { + nsTArray msgs; + nsresult rv = testing::ExtractFromMbox(borkedMbox, msgs, s); + ASSERT_EQ(rv, NS_MSG_ERROR_MBOX_MALFORMED); + } +} diff --git a/mailnews/imap/test/unit/test_downloadOffline.js b/mailnews/imap/test/unit/test_downloadOffline.js index e1c77b1613..ad8657bef9 100644 --- a/mailnews/imap/test/unit/test_downloadOffline.js +++ b/mailnews/imap/test/unit/test_downloadOffline.js @@ -59,3 +59,41 @@ add_task(async function verifyDownloaded() { await streamListener.promise; } }); + +/** + * For mbox, make sure that offline messages fail if the storeTokens + * don't point to the beginning of a message. + */ +add_task( + { + skip_if: () => IMAPPump.inbox.msgStore.storeType != "mbox", + }, + async function checkBadStoreTokens() { + const inbox = IMAPPump.inbox; + + // Corrupt the storeTokens by adding 3. + for (const msg of inbox.messages) { + const offset = Number(msg.storeToken) + 3; + msg.storeToken = offset.toString(); + } + + // Make sure message reading fails. + const NS_MSG_ERROR_MBOX_MALFORMED = 0x80550024; + for (const msg of inbox.messages) { + const streamListener = new PromiseTestUtils.PromiseStreamListener(); + const uri = inbox.getUriForMsg(msg); + const service = MailServices.messageServiceFromURI(uri); + try { + service.streamMessage(uri, streamListener, null, null, false, "", true); + await streamListener.promise; + Assert.ok(false, "Bad storeToken should cause error."); + } catch (e) { + Assert.equal( + e, + NS_MSG_ERROR_MBOX_MALFORMED, + "Bad storeToken causes NS_MSG_ERROR_MBOX_MALFORMED for mbox" + ); + } + } + } +); diff --git a/mailnews/local/test/unit/test_mboxMalformed.js b/mailnews/local/test/unit/test_mboxMalformed.js index e890a8b98e..632f26f678 100644 --- a/mailnews/local/test/unit/test_mboxMalformed.js +++ b/mailnews/local/test/unit/test_mboxMalformed.js @@ -10,6 +10,9 @@ const { PromiseTestUtils } = ChromeUtils.importESModule( "resource://testing-common/mailnews/PromiseTestUtils.sys.mjs" ); +const { MessageGenerator } = ChromeUtils.importESModule( + "resource://testing-common/mailnews/MessageGenerator.sys.mjs" +); // Force mbox mailstore. Services.prefs.setCharPref( @@ -60,3 +63,45 @@ add_task(async function test_unescapedMbox() { // Clear up. localAccountUtils.clearAll(); }); + +/** + * Test that reading from bad offset fails. + */ +add_task(async function test_badStoreTokens() { + localAccountUtils.loadLocalMailAccount(); + const inbox = localAccountUtils.inboxFolder; + + // Add some messages to inbox. + const generator = new MessageGenerator(); + inbox.addMessageBatch( + generator.makeMessages({ count: 10 }).map(m => m.toMessageString()) + ); + + // Corrupt the storeTokens by adding 3 + for (const msg of inbox.messages) { + const offset = Number(msg.storeToken) + 3; + msg.storeToken = offset.toString(); + } + + // Check that message reads fail. + const NS_MSG_ERROR_MBOX_MALFORMED = 0x80550024; + for (const msg of inbox.messages) { + const streamListener = new PromiseTestUtils.PromiseStreamListener(); + const uri = inbox.getUriForMsg(msg); + const service = MailServices.messageServiceFromURI(uri); + + try { + service.streamMessage(uri, streamListener, null, null, false, "", true); + await streamListener.promise; + } catch (e) { + Assert.equal( + e, + NS_MSG_ERROR_MBOX_MALFORMED, + "Bad read causes NS_MSG_ERROR_MBOX_MALFORMED" + ); + } + } + + // Clear up. + localAccountUtils.clearAll(); +});