Closed Bug 1659919 Opened 2 years ago Closed 2 years ago

crash [@ mime_LineBuffer ]

Categories

(Thunderbird :: General, defect)

defect

Tracking

(thunderbird_esr78+ fixed, thunderbird80 wontfix, thunderbird81 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird80 --- wontfix
thunderbird81 --- fixed

People

(Reporter: chriechers, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

(Keywords: regression, regressionwindow-wanted, Whiteboard: smoketestb78.2.0-pre)

Crash Data

Attachments

(1 file, 2 obsolete files)

TB 78.2.0-pre build1, Windows 10, Owl for Exchange 0.7.6

There is one email account set up in the profile connecting to an Exchange Online server via the Owl extension.
Upon attempting to open a PGP/MIME encrypted message sent from another TB78 installation, TB crashes immediately. This is reproducible.
What certainly doesn't help is that Owl does not allow to configure encryption for Exchange accounts. However, decryption always worked with TB68. The corresponding private key is set up in the OpenPGP Key Manager.
In any case, TB shouldn't crash.

crash IDs:
bp-3d565b2b-791e-44a5-8bb0-d42c10200819
bp-56f5cf44-8bd7-4342-aaf9-66bd10200819

https://hg.mozilla.org/releases/comm-esr78/file/tip/mailnews/mime/src/mimebuf.cpp#l141
This is ancient code, so if it regressed it's due to other code changes causing us to hit this case now.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9170873 - Flags: review?(benc)
Comment on attachment 9170873 [details] [diff] [review]
bug1659919_mime_LineBuffer_crash.patch

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

This looks like an improvement, but I don't think it'll solve the problem.

Looking at the code, I think the only reason for the crash is that the buffer is somehow invalid (eg buffer overflow, or pointing to bad memory... or whatever).
So I suspect this patch will just shift the problem elsewhere.

For the bigger picture, stepping back out through the code, it seems that the buffer is coming from a `MimeObject` struct (the `ibuffer_` fields), but it's tricky to follow the thread of it all through various callbacks. So I'd probably be looking at how `MimeObject`  is created and managed.

Ultimately, it'd be nice to be able to replicate the crash in the debugger and see exactly what the state of the buffer is (and also the incoming data). Is there a way I could trigger this crash without requiring an exchange server?

A regression window would probably also give some clues.

So r+, because it's an improvement, but I don't think it's a fix.

::: mailnews/mime/src/mimebuf.cpp
@@ +137,5 @@
>      int32_t* buffer_sizeP, uint32_t* buffer_fpP, bool convert_newlines_p,
>      int32_t (*per_line_fn)(char* line, uint32_t line_length, void* closure),
>      void* closure) {
>    int status = 0;
> +  NS_ASSERTION((uint32_t)*buffer_sizeP > *buffer_fpP, "correct buffer_sizeP");

"buffer is full" as a message, maybe?
Attachment #9170873 - Flags: review?(benc) → review+
Attachment #9170873 - Attachment is obsolete: true
Attachment #9171125 - Flags: review+
OS: Windows 10 → All
Hardware: x86_64 → All
Target Milestone: --- → 81 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/52021a5c2c5b
try to fix crash [@ mime_LineBuffer ]. r=benc

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

The earlier patch caused major problems since I didn't look at the "else case".
So just adding a check to the if-clause instead.

Attachment #9171575 - Flags: review?(benc)
Comment on attachment 9171575 [details] [diff] [review]
bug1659919_linebuffer_crash_v2.patch

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

Looks good to me.
Attachment #9171575 - Flags: review?(benc) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f23742b724d6
try to fix crash [@ mime_LineBuffer ]. r=benc DONTBUILD

Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: 81 Branch → 82 Branch
Attachment #9171125 - Attachment is obsolete: true

Comment on attachment 9171575 [details] [diff] [review]
bug1659919_linebuffer_crash_v2.patch

[Approval Request Comment]
Potential crash fix.

Attachment #9171575 - Flags: approval-comm-esr78?
Attachment #9171575 - Flags: approval-comm-beta?

Comment on attachment 9171575 [details] [diff] [review]
bug1659919_linebuffer_crash_v2.patch

[Triage Comment]
Approved for beta

Attachment #9171575 - Flags: approval-comm-beta? → approval-comm-beta+

I'd like to see this on beta for a short time before trusting on esr. crash ranking is #60, so no rush.

Crash Signature: [@ mime_LineBuffer ]

We saw these crashes related to decrypting via OpenPGP:
https://crash-stats.mozilla.org/report/index/3f1d9924-56a5-4893-a717-f43940200908
https://crash-stats.mozilla.org/report/index/78012e7a-e1ab-43c0-a564-ada440200908
Would be prudent to fix the crash before OpenPGP goes really live.

So I suspect this patch will just shift the problem elsewhere.

This is also true of the newer patch which landed?

For the bigger picture, stepping back out through the code, it seems that the buffer is coming from a MimeObject struct (the ibuffer_ fields), but it's tricky to follow the thread of it all through various callbacks. So I'd probably be looking at how MimeObject is created and managed. Ultimately, it'd be nice to be able to replicate the crash in the debugger and see exactly what the state of the buffer is (and also the incoming data). Is there a way I could trigger this crash without requiring an exchange server?

comment 14 is such a testcase?

bp-3f1d9924-56a5-4893-a717-f43940200908 78.2.2
0 xul.dll mime_LineBuffer(char const*, int, char**, int*, unsigned int*, bool, int ()(char, unsigned int, void*), void*) comm/mailnews/mime/src/mimebuf.cpp:141
1 xul.dll MimeHandleDecryptedOutput(char const*, int, void*) comm/mailnews/mime/src/mimecryp.cpp:264
2 xul.dll nsPgpMimeProxy::OutputDecryptedData(char const*, unsigned int) comm/mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp:392
3 xul.dll XPTC__InvokebyIndex
4 @0x24a18b4b6bf
5 xul.dll truncf
6 xul.dll truncf
7 xul.dll static XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:1140
8 xul.dll XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:946
9 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:585
10 xul.dll Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3312
11 xul.dll js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:465
12 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:620
13 xul.dll js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) js/src/vm/Interpreter.cpp:665
14 xul.dll PromiseReactionJob(JSContext*, unsigned int, JS::Value*) js/src/builtin/Promise.cpp:1906
15 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:585

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benc)

It''s possible the error shifts elsewhere yes. Both patches are needed since the first one was wrong - The combination of the both is correct.
I don't think we have a test case, even if clearly decryption is involved so some cases of that will trigger it.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benc)

Comment on attachment 9171575 [details] [diff] [review]
bug1659919_linebuffer_crash_v2.patch

[Triage Comment]
Approved for esr78

No reports of beta problems thus far, but still bears watching

Attachment #9171575 - Flags: approval-comm-esr78? → approval-comm-esr78+

I have a test message that crashes esr78 in mime_LineBuffer

Applying this patch doesn't solve the problem. Instead, I get a crash soon after.

In my scenario, the problem is that bufferP points to an invalid memory location. The caller is MimeHandleDecryptedOutput, which passes pointers to elements of a MimeObject, but that MimeObject is already an invalid pointer.

Christian, did you already send me a test message?
If not, could you please use the instructions from bug 1663400 to send a test message that triggers this bug?

I found an issue, but I'd like to see which scenarios can trigger it.

Flags: needinfo?(chriechers)

Test message(s) sent from TB78.2.2 on Linux. I also bcc'ed myself, and I can decrypt the message just fine, no crash.

For me the crash happened upon opening the message with TB78.2.1 64-bit on Windows with an Office365 account accessed via the Owl add-on as stated above. It still crashes with TB78.2.2.

Flags: needinfo?(chriechers)

Trying to understand your comment 20:
You say, it can be opened correctly with 78.2.2 on Linux, no crash.
But if you open it with 78.2.2 on Windows 64 bit on Windows, it crashes.
Is this correct?

Flags: needinfo?(chriechers)

Well, sort of.
The message I sent to my work computer (Windows, Office365 account with EWS) did crash Thunderbird right away. This is when I raised the bug.
I did not bcc myself at that time, so I can't tell for sure whether that message would have crashed Thunderbird on Linux, but I doubt it. I've never seen this crash with TB78 on Linux.

The two recent test messages did not crash Thunderbird 78.2.2 for me, neither on Linux nor on Windows with my personal, and normal account (normal meaning IMAP, not Office365 with EWS).

I'd need to send another test message and cc my work account (Windows, Office365 account with EWS), but I can only check that tomorrow.

Flags: needinfo?(chriechers)

STR to produce a crashing message like for this crash:
https://crash-stats.mozilla.org/report/index/95971c6a-928f-4805-9e9b-6f3eb0200916

Send yourself an encrypted message with an image attachment. Just send it to the outbox with Ctrl+Shift+Enter. From there, forward it to yourself as attachment, also encrypted. Also just send to the outbox. On viewing the second message, TB crashes, see report above. No Exchange, Owl, whatever involved.

The 3rd test message crashes TB78.2.2 on Windows immediately (Office365 account with EWS). There's no attachment, just a plain text, PGP/Mime encrypted message.
bp-7eaedb0a-0532-4191-94f7-ae3380200917

There's no crash when opening the message with TB78.2.2 on Linux, and on Windows with a normal (IMAP) account.

No crash for me with Christian's test messages with 78.2.2, tested both Linux 64 bit and Windows 64 bit. (You mentioned a 3rd test message? I only received two at the test accounts.)

No crash for me with the STR from comment 23 with 78.2.2, tested both Linux 64 bit and Windows 64 bit.

However, all stacks in here are the same stack as the crash from bug 1664913 which I can reproduce.
It's a timing and object lifetime issue.

(In reply to Kai Engert (:KaiE:) from comment #26)

No crash for me with Christian's test messages with 78.2.2, tested both Linux 64 bit and Windows 64 bit. (You mentioned a 3rd test message? I only > received two at the test accounts.)

I did not send the 3rd test message to the test account, as the previous two messages already happened to work.
The presence of the Office365 account and the Owl add-on may just be coincidence, and not related at all. But then I don't know what's causing that particular Thunderbird installation to crash. All I can say is that the crash is exactly reproducible with that installation.

Ok, I'm finally able to reproduce using the steps from this bug, too.

In addition to having a forwarded/attached encrypted message, it's also necessary that the second message contains an additional attachment, like your public key.

This is the same cause as bug 1665475, and I have a fix that works around the crash.

Resolution: FIXED → DUPLICATE
Duplicate of bug: 1665475

(let's keep it "fixed" since a fix was landed)

Resolution: DUPLICATE → FIXED

FYI, fixes have been pushed to c-c with bug number 1665475

See Also: → 1665475
You need to log in before you can comment on or make changes to this bug.