1539028 - Crash in [@ ReleaseObjects ] or [@ nsCOMArray_base::Clear] via nsMsgSendLater::EndSendMessages UAF

Status: NEW

(MailNews Core :: Composition, defect)

Description

[Wayne Mery (:wsmwk)]

I had just sent an email. I have background send enabled.

This bug is for crash report bp-445bd6ee-f26a-4d6e-a725-b64d10190325.

Top 10 frames of crashing thread:

0 @0x3aed28 
1 xul.dll nsCOMArray_base::Clear xpcom/ds/nsCOMArray.cpp:237
2 xul.dll nsMsgSendLater::EndSendMessages comm/mailnews/compose/src/nsMsgSendLater.cpp:1339
3 xul.dll nsMsgSendLater::StartNextMailFileSend comm/mailnews/compose/src/nsMsgSendLater.cpp:594
4 xul.dll nsMsgSendLater::OnCopyStepFinished comm/mailnews/compose/src/nsMsgSendLater.cpp:1407
5 xul.dll SendOperationListener::OnStopCopy comm/mailnews/compose/src/nsMsgSendLater.cpp:503
6 xul.dll nsMsgComposeAndSend::MaybePerformSecondFCC comm/mailnews/compose/src/nsMsgSend.cpp:4040
7 xul.dll nsMsgComposeAndSend::OnStopOperation comm/mailnews/compose/src/nsMsgSend.cpp:3999
8 xul.dll nsMsgFilterAfterTheFact::OnEndExecution comm/mailnews/base/search/src/nsMsgFilterService.cpp:356
9 xul.dll nsMsgFilterAfterTheFact::AdvanceToNextFolder comm/mailnews/base/search/src/nsMsgFilterService.cpp:426

(This issue is distinct from bug 1454170 - Crash in nsCOMArray_base::Clear during SnowWhiteKiller cycle collection (Mac))

Comment 1

[Magnus Melin [:mkmelin]]

https://dxr.mozilla.org/mozilla-esr60/source/xpcom/ds/nsCOMArray.cpp?q=nsCOMArray.cpp&redirect_type=direct#237
https://hg.mozilla.org/releases/comm-esr60/file/ec1b1f6de68c18251ec3699b88dd4575ffdda628/mailnews/compose/src/nsMsgSendLater.cpp#l1339
https://hg.mozilla.org/releases/comm-esr60/file/ec1b1f6de68c18251ec3699b88dd4575ffdda628/mailnews/compose/src/nsMsgSendLater.cpp#l594

...under which there is a comment "// XXX Should we be releasing references so that we don't hold onto items unnecessarily."

Comment 2

[Wayne Mery (:wsmwk)]

Ben, does comment 1 make sense for bp-68ec0254-a0c2-4e78-9985-1c9620200911 where we don't end up in SnowWhiteKiller ...

as opposed to bp-8e928244-e0b7-4d91-bc9a-26a0d0200911 where we do.

Comment 3

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #1)

https://hg.mozilla.org/releases/comm-esr60/file/ec1b1f6de68c18251ec3699b88dd4575ffdda628/mailnews/compose/src/nsMsgSendLater.cpp#l594
...under which there is a comment "// XXX Should we be releasing references so that we don't hold onto items unnecessarily."

It's a valid comment - looking at nsMsgSendLater is does seem that is the odd member variable which could hold a reference longer than stricty required (eg mMessage could be cleared when that send is complete). But I can't see that causing the crash.

(In reply to Wayne Mery (:wsmwk) from comment #2)

Ben, does comment 1 make sense for bp-68ec0254-a0c2-4e78-9985-1c9620200911 where we don't end up in SnowWhiteKiller ...
as opposed to bp-8e928244-e0b7-4d91-bc9a-26a0d0200911 where we do.

What is interesting is the crash address for the reports.

So many of them crash trying to read 0xe5e5e5ed. The fill pattern for freed memory is 0xe5e5e5e5, so it rather looks like somewhere is trying to add 8 to a pointer residing in freed memory, and then attempting to dereference it.

My first impulse is that the nsMsgSendLater and SnowWhiteKiller crashes weren't related, but the 0xe5e5e5ed seems very suggestive that they are related - if not in cause, then by committing the same error: performing an operation on something after it's been freed.

That's about all I've got :-( Best current guess: freeing something on another thread while there's still an event queued up to use it...

Comment 4

[Wayne Mery (:wsmwk)]

Perhaps a variation of the exceptionally common Bug 1517464 - Thunderbird crash in nsXPCWrappedJS::Release sending mail (or saving to Sent or Drafts)

FWIW, we have these other "send later" issues":

• Bug 888755 - convert nsMsgSendLater::mLeftoverBuffer to nsTArray<char>
• Bug 1088152 - Clean up shutdown observers in nsMsgSendLater
• Bug 1606127 - Intermittent comm/mailnews/compose/test/unit/test_sendMessageLater3.js | xpcshell return code: 0

Comment 5

[Wayne Mery (:wsmwk)]

crash rate Windows is one fourth what it was 6 months ago. (like bug 1454170 for Mac)

bp-16d2a371-952d-4325-988f-d53870201231 Windows Crash Address 0xffffffffffffffff
bp-92fea742-88c5-4877-8c53-9c9370201231 Windows Crash Address 0xe5e5e5ed

Comment 7

[Wayne Mery (:wsmwk)]

The addresses mentioned in Ben's comment 3 are still predominant. Perhaps this correlates to bug 1508649.

bp-426bf9dc-3683-4e0a-bf44-5dd630210905 78.13.0
bp-466e59bc-b031-43bf-9d4a-7cb500210905 78.13.0
bp-be6bd493-b615-4b20-891b-c3e060210903 78.13.0
bp-a73caee8-cda4-43f8-b321-bfc640210902 91.0.3

Comment 8

[Wayne Mery (:wsmwk)]

Magnus, what makes sense as a next step? Anything in comment 4 and comment 3?

Approx 1/3 of crashes have nsMsgSendLater::EndSendMessages on the stack

bp-536d35d7-cd16-4ccf-af51-8c22f0211031 is one that does not have nsMsgSendLater::EndSendMessages on the stack

Comment 9

[Magnus Melin [:mkmelin]]

Maybe somehow null is getting into the mMessagesToSend?
https://searchfox.org/comm-central/rev/5e94c611d13ff79f35b34f91c13af7064a927d58/mailnews/compose/src/nsMsgSendLater.cpp#720
https://searchfox.org/comm-central/rev/5e94c611d13ff79f35b34f91c13af7064a927d58/mailnews/base/src/nsMsgEnumerator.cpp#103,114

Comment 10

[Wayne Mery (:wsmwk)]

Roughly still same crash rate for version 102.
bp-bdad171a-629f-4e97-b257-0fcf70230115

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Magnus Melin [:mkmelin] from comment #9)

Maybe somehow null is getting into the mMessagesToSend?
https://searchfox.org/comm-central/rev/5e94c611d13ff79f35b34f91c13af7064a927d58/mailnews/compose/src/nsMsgSendLater.cpp#720
https://searchfox.org/comm-central/rev/5e94c611d13ff79f35b34f91c13af7064a927d58/mailnews/base/src/nsMsgEnumerator.cpp#103,114

Comment 12

[Ben Campbell]

Looking at bp-bdad171a-629f-4e97-b257-0fcf70230115, it's crashing trying to access 0xe5e5e5e5e5e5e5f5. This looks it's accessing a pointer in freed memory (So the pointer would be 0xe5e5e5e5e5e5e5e5, but adding an offset of 0x10, maybe to handle inheritance and get to the vtable?)

The order of calls is:

1. nsMsgDBView::Release()
2. nsMsgXFVirtualFolderDBView::~nsMsgXFVirtualFolderDBView()
3. nsCOMArray_base::Clear()
4. nsCOMArray_base::~nsCOMArray_base()
5. kaboom.

~nsMsgXFVirtualFolderDBView() is empty, and I'd guess Clear() is inlined...
so my guess would be that nsMsgXFVirtualFolderDBView was already freed.
But it's unclear who owns it. Stack trace seems to indicate that it's being freed as part of some sort of cyclic-reference cleanup in JS land?
Kind of out of ideas after that. I imagine there's somewhere in the C++ side which just isn't being careful enough with it refcounting of nsMsgDBViews... but that's a pretty big surface area :-(

Comment 13

[Wayne Mery (:wsmwk)]

In version 115 this might now be ReleaseObjects
https://crash-stats.mozilla.org/signature/?proto_signature=~nsMsgSendLater%3A%3AEndSendMessages&product=Thunderbird&signature=ReleaseObjects&date=%3E%3D2023-06-30T13%3A43%3A00.000Z&date=%3C2023-12-31T13%3A43%3A00.000Z#graphs

bp-468fcb97-d026-485b-86a6-bc0a90231229 Crash Address 0xe5e5e5e5e5e5e5f5
bp-4c5855be-a6df-48e9-a937-2056d0231230 Crash Address 0x0000000100000011