1864344 - Crash in [@ nsSaveMsgListener::OnStopRequest] use after free (UAF)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

Crashes go back at least as far as Thunderbird 114 beta. All have crash address of 0xe5e5e5e5. #85 crash for Thunderbird 115.4.2

No Firefox crashes with this signature.

Crash report: https://crash-stats.mozilla.org/report/index/14cfaa56-fb21-4f07-8405-694640231112

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsSaveMsgListener::OnStopRequest  mailnews/base/src/nsMessenger.cpp:1523
1  xul.dll  nsStreamConverter::OnStopRequest  mailnews/mime/src/nsStreamConverter.cpp:906
2  xul.dll  nsImapCacheStreamListener::OnStopRequest  mailnews/imap/src/nsImapProtocol.cpp:8789
3  xul.dll  nsInputStreamPump::OnStateStop  netwerk/base/nsInputStreamPump.cpp:695
4  xul.dll  nsInputStreamPump::OnInputStreamReady  netwerk/base/nsInputStreamPump.cpp:415
5  xul.dll  CallbackHolder::CallbackHolder::<lambda_1>::operator const  xpcom/io/nsPipe3.cpp:73
5  xul.dll  NS_NewCancelableRunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/io/nsPipe3.cpp:71:35'>::FuncCancelableRunnable::Run  xpcom/threads/nsThreadUtils.h:667
6  xul.dll  mozilla::RunnableTask::Run  xpcom/threads/TaskController.cpp:555
7  xul.dll  mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal  xpcom/threads/TaskController.cpp:879
8  xul.dll  mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal  xpcom/threads/TaskController.cpp:702

Comment 1

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/6a0aebcb94ac87828941e1c53483d2435aeedc44/mailnews/base/src/nsMessenger.cpp#1484,1523,1525-1526
delete and then set to nullptr? Seems fishy but my c++ foo is not strong enough to say if/what should be done here.

Comment 2

[Ben Campbell]

Ahh. Think I've figured it out, and identified the culprit responsible.

The internal SaveAttachment() usually passes the nsSaveAllAttachmentsState on to the next SaveAttachment() call. It's icky.
But some code was added so that if SaveAttachment() fails, it deletes the state object. Which kind of makes sense - the SaveAttachment() function is the only thing that really owns the state object. I guess before then, the state object just leaked upon error...

https://searchfox.org/comm-central/rev/6a0aebcb94ac87828941e1c53483d2435aeedc44/mailnews/base/src/nsMessenger.cpp#539

BUT... the OnStopRequest() call also deletes the state object if anything goes wrong. Including a failed SaveAttachment(), which means there'll be a double-delete. Uhoh.

The patch that introduced the problem is https://phabricator.services.mozilla.com/D157065 (from Bug 1786237).

I think the right solution is to make SaveAttachment() leave the state as-is if it fails, and change all the places that call it to perform the deletion instead... I'll work up a patch to do that.

It all needs a good cleanout (See Bug 1788159).

Comment 3

[Wayne Mery (:wsmwk)]

Thanks for the info.

Comment 4

[Ben Campbell]

Attachment: Bug 1864344 - Prevent use-after-free bug in nsSaveMsgListener::OnStopRequest(). r=#thunderbird-reviewers

This is very much just a quick fix to paper over this specific crash bug.
The proper solution is to rewrite the whole attachment-saving system.
See Bug 1788159.

Comment 5

[Wayne Mery (:wsmwk)]

Thanks for the patch.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8b3c2a9fc8a2
Prevent use-after-free bug in nsSaveMsgListener::OnStopRequest(). r=mkmelin

Comment 7

[Wayne Mery (:wsmwk)]

good for 115?

Comment 8

[Ben Campbell]

Yes, I think it'll be fine for 115

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9364402 [details]
Bug 1864344 - Prevent use-after-free bug in nsSaveMsgListener::OnStopRequest(). r=#thunderbird-reviewers

[Triage Comment]
Approved for esr115

Comment 10

[Rob Lemley [:rjl]]

Thunderbird 115.5.2:
https://hg.mozilla.org/releases/comm-esr115/rev/3f0d790b0cce

Comment 11

[Wayne Mery (:wsmwk)]

So far no crashes for 115.5.2.
https://crash-stats.mozilla.org/signature/?signature=nsSaveMsgListener%3A%3AOnStopRequest&date=%3E%3D2023-11-13T00%3A00%3A00.000Z&date=%3C2023-12-13T23%3A59%3A00.000Z