877831 - crash [@ nsParseMailMessageState::FinalizeHeaders()], [libSystem.B.dylib@0x4123b | ParseString | nsParseMailMessageState::FinalizeHeaders ] (5.0, Mac )

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #669136 +++

despite the patch from Bug #669136, fixed in TB 17.0.3? ...
we still have in TB 17.0.6 mozalloc_abort(char const* const) | NS_DebugBreak_P | nsACString_internal::Assign(nsACString_internal const&)  
I did not check for other crash signatures
ranked #46

I have not assessed what impact Bug #669136 had, if any.

for example bp-f0004fb5-edcc-4cd9-843d-7fbb02130516 TB17.0.6
0	mozalloc.dll	mozalloc_abort	memory/mozalloc/mozalloc_abort.cpp:23
1	xul.dll	NS_DebugBreak_P	xpcom/base/nsDebugImpl.cpp:410
2	xul.dll	nsACString_internal::Assign	xpcom/string/src/nsTSubstring.cpp:348
3	xul.dll	mozilla::ipc::StringInputStreamParams::StringInputStreamParams	objdir-tb/mozilla/dist/include/nsTString.h:46
4	xul.dll	nsTArray<mozilla::dom::indexedDB::Key,nsTArrayInfallibleAllocator>::AssignRange<	objdir-tb/mozilla/dist/include/nsTArray.h:1238
5	xul.dll	nsTArray<nsCString,nsTArrayDefaultAllocator>::AppendElements<nsACString_internal	objdir-tb/mozilla/dist/include/nsTArray.h:882
6	xul.dll	ParseString	xpcom/string/src/nsReadableUtils.cpp:709
7	xul.dll	nsParseMailMessageState::FinalizeHeaders	mailnews/local/src/nsParseMailbox.cpp:1607
8	xul.dll	nsParseMailMessageState::ParseFolderLine	mailnews/local/src/nsParseMailbox.cpp:675
9	xul.dll	nsMsgMailboxParser::HandleLine	mailnews/local/src/nsParseMailbox.cpp:490
10	xul.dll	nsMsgLineBuffer::ConvertAndSendBuffer	mailnews/base/util/nsMsgLineBuffer.cpp:236
11	xul.dll	nsMsgLineBuffer::BufferInput	mailnews/base/util/nsMsgLineBuffer.cpp:173
12	xul.dll	nsMsgMailboxParser::ProcessMailboxInputStream	mailnews/local/src/nsParseMailbox.cpp:333 

same user as above, bp-a7742b3c-58a1-4595-bfe4-862e82130516  TB17.0.6
different user bp-444a68e4-104a-48a4-8a2e-82af82130506  TB17.0.5

Comment 1

[Hiroyuki Ikezoe (:hiro)]

(In reply to Wayne Mery (:wsmwk) from comment #0)
> +++ This bug was initially created as a clone of Bug #669136 +++
> 
> despite the patch from Bug #669136, fixed in TB 17.0.3? ...
> we still have in TB 17.0.6 mozalloc_abort(char const* const) |
> NS_DebugBreak_P | nsACString_internal::Assign(nsACString_internal const&)  
> I did not check for other crash signatures
> ranked #46

As far as I remember, the patch committed in bug #669136 is not exactly fix for bug #669136. The patch is for bug #791149.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix with a unit test

The unit test has invalid X-Mozilla-Keys header. I have no idea how Thunderbird creates the header so I created the header by hand.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Additional protection against crash

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 758928 [details] [diff] [review]
Additional protection against crash

I would recommend to apply this patch for the safety.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

My mercurial account seems to be disabled, someone please push these patches on try server on behalf of me.
Thank you.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Pushed two patches to try-comm-central: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1fe5283a23cb

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix crash of unit test in attachment 758927 on debug build

Irving, thank you for you kindness.

The try server results noticed me that NS_ASSERTION in nsParseMailMessageState::GetAggregateHeader causes crash on debug build.
This patch removes the problematic NS_ASSERTION in nsParseMailMessageState::GetAggregateHeader since strlen is useless there.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

For the record, the crash log from the try result:

###!!! ASSERTION: header corrupted: 'header->length == (int32_t)strlen(header->value)', file ../../../../mailnews/local/src/nsParseMailbox.cpp, line 877
<<<<<<<
PROCESS-CRASH | /home/cltbld/talos-slave/test/build/xpcshell/tests/mailnews/local/test/unit/test_nsIMsgParseMailMsgState.js | application crashed [@ mozalloc_abort(char const*)]
Crash dump filename: /home/cltbld/talos-slave/test/build/xpcshell/tests/mailnews/local/test/unit/70e52d4b-a1b3-3c56-29565fc0-69f2ca46.dmp

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 758927 [details] [diff] [review]
Fix with a unit test

These patches are failing on my machine running Mac debug. I'm seeing:

TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/mailnews/local/test/unit/test_nsIMsgParseMailMsgState.js | test failed (with xpcshell return code: 1), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpFsiTCj/runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
Directory request for: MailD that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: DefRt that we (mailDirService.js) are not handling, leaving it to another handler.
###!!! ASSERTION: subject corrupt while parsing message: 'header->length == (short) strlen(header->value)', file /Users/moztest/comm/main/src/mailnews/local/src/nsParseMailbox.cpp, line 1219
nsParseMailMessageState::FinalizeHeaders()+0x00000EB6 [/Users/moztest/comm/main/tb/mozilla/dist/bin/XUL +0x017D5E46]
nsParseMailMessageState::ParseFolderLine(char const*, unsigned int)+0x0000004C [/Users/moztest/comm/main/tb/mozilla/dist/bin/XUL +0x017D416C]
nsParseMailMessageState::ParseAFolderLine(char const*, unsigned int)+0x0000000D [/Users/moztest/comm/main/tb/mozilla/dist/bin/XUL +0x017D410D]

Given what these are testing, I don't think it is worth me trying to review the individual patches. Once you've fixed that, can you put all the patches in one, unless there's a specific reason not to.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Update patch

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 766955 [details] [diff] [review]
Update patch

Review of attachment 766955 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/local/src/nsParseMailbox.cpp
@@ -1445,1 @@
>          ch = PL_strchr(recipient->value, ',');

Is strlen usless because receipent->value isn't null-terminated? If so, shoudn't we be fixing PR_strchr as well, which relies on a null-terminated value?

If that's the case, please provide a new patch. I'll examine for other instances on the new patch.

If not, please re-request review with an explanation.

Thanks.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 766955 [details] [diff] [review]
Update patch

(In reply to Mark Banner (:standard8) from comment #11)
> Comment on attachment 766955 [details] [diff] [review]
> Update patch
> 
> Review of attachment 766955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/local/src/nsParseMailbox.cpp
> @@ -1445,1 @@
> >          ch = PL_strchr(recipient->value, ',');
> 
> Is strlen usless because receipent->value isn't null-terminated? 

Right. But I prefer the patch for this crash bug contains exactly the fix for the crash. Fix contained other instances will be bigger, then the fix will be blur fix for this crash.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

1207 nsresult nsParseMailMessageState::InternSubject (struct message_header *header)
1208 {
1209   char *key;
1210   uint32_t L;
1211 
1212   if (!header || header->length == 0)
1213   {
1214     m_newMsgHdr->SetSubject("");
1215     return NS_OK;
1216   }
1217 
1218   NS_ASSERTION (header->length == (short) strlen(header->value), "subject corrupt while parsing message");
1219 
1220   key = (char *) header->value;  /* #### const evilness */
1221 
1222   L = header->length;
1223 
1224 
1225   uint32_t flags;
1226   (void)m_newMsgHdr->GetFlags(&flags);
1227   /* strip "Re: " */
1228   /**
1229         We trust the X-Mozilla-Status line to be the smartest in almost
1230         all things.  One exception, however, is the HAS_RE flag.  Since
1231          we just parsed the subject header anyway, we expect that parsing
1232          to be smartest.  (After all, what if someone just went in and
1233         edited the subject line by hand?)
1234      */
1235   nsCString modifiedSubject;
1236   if (NS_MsgStripRE((const char **) &key, &L, getter_Copies(modifiedSubject)))
1237     flags |= nsMsgMessageFlags::HasRe;
1238   else
1239     flags &= ~nsMsgMessageFlags::HasRe;
1240   m_newMsgHdr->SetFlags(flags); // this *does not* update the mozilla-status header in the local folder
1241 
1242   //  if (!*key) return 0; /* To catch a subject of "Re:" */
1243 
1244   // Condense the subject text into as few MIME-2 encoded words as possible.
1245 #ifdef WE_CONDENSE_MIME_STRINGS
1246   char *condensedKey = msg_condense_mime2_string(modifiedSubject.IsEmpty() ? key : modifiedSubject.get());
1247 #else
1248   char *condensedKey = nullptr;
1249 #endif
1250   m_newMsgHdr->SetSubject(condensedKey ? condensedKey :
1251   (modifiedSubject.IsEmpty() ? key : modifiedSubject.get()));
1252   PR_FREEIF(condensedKey);
1253 
1254   return NS_OK;
1255 }
 
I am suspecting this 'key', which should be handled as non-null-terminated string, causes bug 857678.
The fix for this part should be separated.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

After investigating more carefully, there are lots of instances which should be handled as non-null terminated. For examples:

1371         rawMsgId.Assign(id->value);
1416         m_newMsgHdr->SetStringProperty("replyTo", replyTo->value);
1433         m_newMsgHdr->SetAuthor(sender->value);
... and many.

I would like to fix those instances in another bug.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/74ec5a627dcc

Comment 16

[Wayne Mery (:wsmwk)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> After investigating more carefully, there are lots of instances which should
> be handled as non-null terminated. For examples:
> 
> 1371         rawMsgId.Assign(id->value);
> 1416         m_newMsgHdr->SetStringProperty("replyTo", replyTo->value);
> 1433         m_newMsgHdr->SetAuthor(sender->value);
> ... and many.
> 
> I would like to fix those instances in another bug.

Thanks Hiro. Can you file the necessary bugs?

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 766955 [details] [diff] [review]
Update patch

[Triage Comment]
Ok, this still seems to happen occasionally, so lets take it onto 24.

Comment 18

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr24/rev/1f92c1a7ec21

Comment 19

[Wayne Mery (:wsmwk)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> After investigating more carefully, there are lots of instances which should
> be handled as non-null terminated. For examples:
> 
> 1371         rawMsgId.Assign(id->value);
> 1416         m_newMsgHdr->SetStringProperty("replyTo", replyTo->value);
> 1433         m_newMsgHdr->SetAuthor(sender->value);
> ... and many.
> 
> I would like to fix those instances in another bug.

I have filed bug 1051616, and there is an older bug 791149, where this work can continue

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> After investigating more carefully, there are lots of instances which should
> be handled as non-null terminated. For examples:
> 
> 1371         rawMsgId.Assign(id->value);
> 1416         m_newMsgHdr->SetStringProperty("replyTo", replyTo->value);
> 1433         m_newMsgHdr->SetAuthor(sender->value);
> ... and many.

All non-null terminated string have been fixed by the patch for bug 973316.