1586890 - Crash in [@ MimeInlineTextPlain_parse_line]

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Wayne Mery (:wsmwk)]

This signature appears to be a regression with first crash report being buildid 20190930082403 bp-ff7c3b34-696b-4e46-95d4-e1afb0190930.

Top 10 frames of crashing thread:
0 xul.dll static int MimeInlineTextPlain_parse_line comm/mailnews/mime/src/mimetpla.cpp
1 xul.dll static int MimeInlineText_convert_and_parse_line comm/mailnews/mime/src/mimetext.cpp:335
2 xul.dll mime_LineBuffer comm/mailnews/mime/src/mimebuf.cpp:149
3 xul.dll static int MimeInlineText_parse_decoded_buffer comm/mailnews/mime/src/mimetext.cpp:285
4 xul.dll MimeDecoderWrite comm/mailnews/mime/src/mimeenc.cpp
5 xul.dll static int MimeLeaf_parse_buffer comm/mailnews/mime/src/mimeleaf.cpp:139
6 xul.dll mime_LineBuffer comm/mailnews/mime/src/mimebuf.cpp:207
7 xul.dll static int MimeObject_parse_buffer comm/mailnews/mime/src/mimeobj.cpp:223
8 xul.dll mime_display_stream_write comm/mailnews/mime/src/mimemoz2.cpp:853
9 xul.dll nsStreamConverter::OnDataAvailable comm/mailnews/mime/src/nsStreamConverter.cpp:825

Comment 1

[Jorg K (CEST = GMT+2)]

Well, if I believe the picture, there were crashes starting in May 2019.

This code hasn't changed in ages:
https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimetpla.cpp
https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimetext.cpp

The crash is a MOZ_DIAGNOSTIC_ASSERT() that we don't pass anything silly into nsTDependentString() here:
https://searchfox.org/comm-central/rev/6c35d7efb98e7903b3e77723770390a246b92fd5/mailnews/mime/src/mimetpla.cpp#288

And the MOZ_DIAGNOSTIC_ASSERT() was already added in March 2019 here:
https://hg.mozilla.org/mozilla-central/rev/eeca0e099b8a#l1.21

So what's going on?

We could do something like we've done here: https://hg.mozilla.org/comm-central/rev/57dccd17e67f but I'd really like to understand what's going on.

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) (PTO to 15th Dec 2019, sporadically reading bugmail) from comment #1)

We could do something like we've done here: https://hg.mozilla.org/comm-central/rev/57dccd17e67f but I'd really like to understand what's going on.

Jorg, to follow up ... I was mainly concerned that we might have a clear bad regression or something easy to fix. The bad regression didn't materialize so I'm happy to closed if there's no obvious fix or this isn't worth keeping open, because there are almost no crashes (unless there are other crash signatures derived from the same issue) https://crash-stats.mozilla.org/signature/?signature=MimeInlineTextPlain_parse_line&date=%3E%3D2019-06-09T18%3A58%3A00.000Z&date=%3C2019-12-09T18%3A58%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#graphs

Comment 3

[Alfred Peters [:infofrommozilla]]

Curiously I got an crash signature witch leads me to this bug:

bp-09f54661-0299-403c-8ff2-4af520200826

But my regression window is relative new:

Last good:
Thunderbird (81.0a1) - Thunderbird Daily BuildID=20200819093243
Rev: Comm-Central:17e03dc81d0d / Mozilla-Central:157db696462d

First bad:
Thunderbird (81.0a1) - Thunderbird Daily BuildID=20200820134908
Rev: Comm-Central:c2e7b598699a / Mozilla-Central:8cb700c12bd3

STR:

• Enable "Display Attachments Inline"
• Set pref: mail.inline_attachments.text
• select an email with an test attachment of type UTF-16LE or UTF-16BE with BOM

Comment 4

[Alfred Peters [:infofrommozilla]]

Attachment: Email with attachment; name="utf-16le+bom.txt" charset=utf-16le;

Comment 5

[Alfred Peters [:infofrommozilla]]

Attachment: Email with attachment; name="utf-16be+bom.txt" charset=utf-16be;

Comment 6

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #3)

bp-09f54661-0299-403c-8ff2-4af520200826

But my regression window is relative new:

Last good:
Thunderbird (81.0a1) - Thunderbird Daily BuildID=20200819093243
Rev: Comm-Central:17e03dc81d0d / Mozilla-Central:157db696462d

First bad:
Thunderbird (81.0a1) - Thunderbird Daily BuildID=20200820134908
Rev: Comm-Central:c2e7b598699a / Mozilla-Central:8cb700c12bd3

Finally it's Bug 1440677
https://hg.mozilla.org/mozilla-central/rev/d1f68e04245156b28258bed007957176bd4ab4ec

Comment 7

[:Gijs (he/him)]

(In reply to Alfred Peters from comment #6)

(In reply to Alfred Peters from comment #3)

bp-09f54661-0299-403c-8ff2-4af520200826

But my regression window is relative new:

Last good:
Thunderbird (81.0a1) - Thunderbird Daily BuildID=20200819093243
Rev: Comm-Central:17e03dc81d0d / Mozilla-Central:157db696462d

First bad:
Thunderbird (81.0a1) - Thunderbird Daily BuildID=20200820134908
Rev: Comm-Central:c2e7b598699a / Mozilla-Central:8cb700c12bd3

Finally it's Bug 1440677
https://hg.mozilla.org/mozilla-central/rev/d1f68e04245156b28258bed007957176bd4ab4ec

This is an m-c change that didn't change the mailnews parsing code that's crashing in the stacks reported here...

Perhaps the change in parameterized headers parsing causes the already-broken mailnews code to be hit in cases where it wasn't before, or with content it's not expecting? Or perhaps I'm missing some way that parse_line is interacting with that code that isn't obvious from a casual look?

The attachment has:

Subject: Content-Disposition: attachment; name="utf-16be+bom.txt" charset=utf-16be;

did you perhaps edit the subject out, or something?

Also, isn't there meant to be a ; between the pairs of name=value?

Comment 8

[Alfred Peters [:infofrommozilla]]

(In reply to :Gijs (he/him) from comment #7)

(In reply to Alfred Peters from comment #6)

Perhaps the change in parameterized headers parsing causes the already-broken mailnews code to be hit in cases where it wasn't before, or with content it's not expecting? Or perhaps I'm missing some way that parse_line is interacting with that code that isn't obvious from a casual look?

Yes, maybe.

The attachment has:

Subject: Content-Disposition: attachment; name="utf-16be+bom.txt" charset=utf-16be;

did you perhaps edit the subject out, or something?

Yes, these are some of my test cases. Originally it was one mail with seven attachments.

But:

Also, isn't there meant to be a ; between the pairs of name=value?

Good find.

--------------7A8C3064E8E211D0817C5155
Content-Type: text/plain; charset=utf-16le
 name="utf-16le+bom.txt"

There is the semicolon missing after the charset parameter. With the ; the crash is gone.

But that doesn't change the fact that TB must never crash even with malicious data.

Comment 9

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #8)

(In reply to :Gijs (he/him) from comment #7)

--------------7A8C3064E8E211D0817C5155
Content-Type: text/plain; charset=utf-16le
 name="utf-16le+bom.txt"

There is the semicolon missing after the charset parameter. With the ; the crash is gone.

Yes, because of Bug 1440677 anything till the next ; or lineend will land into the parameter. Therfore charset will become:
utf-16le name="utf-16le+bom.txt"

Guess:
Now TB doesn't recognize the 16-bit charset. The 0-bytes inside the body now lead to the crash.

Proof:

Content-Type: text/plain; charset=FooBar;  name="utf-16le+bom.txt"
Content-Transfer-Encoding: base64

=> crash

Content-Type: text/plain; charset=FooBar;  name="utf-16le+bom.txt"
Content-Transfer-Encoding: 8bit

=> NO crash

If that's right, a false charset like "us-ascii", "utf-8" or "windows-1252" should also lead to a crash, but they don't.

Comment 10

[:Gijs (he/him)]

So, fwiw, I'm not in a position to fix TB here, mostly because I don't have a setup to actually build the native parts of comm-central, but also because I just don't have time. :-(

It seems like this crash pre-dates the regressor in general, but the changes probably make it easier to hit? Though the spikes in crash-stats also predate the regressor. So I dunno - it sounds like (a) the code in general needs to be able to deal with the charset being misparsed / containing null bytes (at least not crash) and (b) perhaps TB wants different parsing of these "header" values, if mail clients use different invalid headers from web servers (sigh). Perhaps that can be conditioned on the aEncoding value passed into netwerk's mime header parser; perhaps the parser should be forked. I don't know. I'm happy to help with questions, but unfortunately I can't chase fixing this myself.

Comment 11

[Alfred Peters [:infofrommozilla]]

Attachment: Fix crash on BASE64 encoded utf-16 text with invalid charset.

Short story?
My debugger stopped at this assertion:
https://dxr.mozilla.org/comm-central/source/xpcom/string/nsTString.h#490
It suggests to use nsTDependentSubstring instead of nsTDependentString because ther is no terminating 0
Indeed, our BASE64 decoder gives back a string without 0-termination.
I've done so - Crash is gone.

I'm not sure who wants to review? Magnus Melin?

Comment 12

[Alfred Peters [:infofrommozilla]]

Long story?
Our BASE64 decoder is broken for 16-bit charsets (see Bug 1642917 Bug 244829 etc.).
I don't really know, why it doesn't crash when the charset is valid. I guess then there is a converter that handles the sting differently. But I didn't trace that case.

Comment 13

[Alfred Peters [:infofrommozilla]]

(In reply to :Gijs (he/him) from comment #10)

It seems like this crash pre-dates the regressor in general, but the changes probably make it easier to hit? Though the spikes in crash-stats also predate the regressor. So I dunno - it sounds like (a) the code in general needs to be able to deal with the charset being misparsed / containing null bytes (at least not crash) and (b) perhaps TB wants different parsing of these "header" values, if mail clients use different invalid headers from web servers (sigh). Perhaps that can be conditioned on the aEncoding value passed into netwerk's mime header parser; perhaps the parser should be forked.

Regardless of the crash, this is a point to reconsider. We should keep this in mind if the next email is displayed incorrectly.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9172759 [details] [diff] [review]
Fix crash on BASE64 encoded utf-16 text with invalid charset.

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

Thanks! Can we add a sample problem message to use in the tests? 
Maybe just add it as one of the messages to search for in https://searchfox.org/comm-central/source/mailnews/search/test/unit/test_searchBody.js ?

Comment 15

[Alfred Peters [:infofrommozilla]]

Attachment: 9172759: Fix crash on BASE64 encoded utf-16 text with invalid charset.

Our BASE64 decoder gives back a string without 0-termination. Therefor nsTDependentCSubstring is needed instead of nsTDependentCString.

I added the test to browser_base64Display.js.

Nit: I changed the message of the old Test.
Old: PASS Decoded base64 text not found in message.
Why did it pass when the text wasn't found??? That confused me a bit ;-)
New PASS Decoded base64 body from message.

Comment 16

[Alfred Peters [:infofrommozilla]]

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

Thanks! Can we add a sample problem message to use in the tests?
Maybe just add it as one of the messages to search for in
https://searchfox.org/comm-central/source/mailnews/search/test/unit/
test_searchBody.js ?

test_searchBody.js won't work because the search uses a different code path / BASE64 decoder.
Therefor I put it into browser_base64Display.js.

Comment 17

[Alfred Peters [:infofrommozilla]]

Attachment: 9173071: 9172759: Fix crash on BASE64 encoded utf-16 text with invalid charset.

Our BASE64 decoder gives back a string without 0-termination. Therefor nsTDependentCSubstring is needed instead of nsTDependentCString.

I added the test to browser_base64Display.js.

Nit: I changed the message of the old Test.
Old: PASS Decoded base64 text not found in message.
Why did it pass when the text wasn't found??? That confused me a bit ;-)
New PASS Decode base64 body from message.

[Edit:] Sorry - Spelling typo

Comment 18

[Ben Campbell]

Comment on attachment 9173082 [details] [diff] [review]
9173071: 9172759: Fix crash on BASE64 encoded utf-16 text with invalid charset.

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

Looks good to me!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4342d28b162d
Fix crash on BASE64 encoded utf-16 text with invalid charset. r=benc

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9c27ed79f66
follow-up - Fix linter error. rs=linting

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9173082 [details] [diff] [review]
9173071: 9172759: Fix crash on BASE64 encoded utf-16 text with invalid charset.

[Approval Request Comment]
Crash fix, has automated test.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9173082 [details] [diff] [review]
9173071: 9172759: Fix crash on BASE64 encoded utf-16 text with invalid charset.

[Approval Request Comment]
Crash fix, has automated test.

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9173082 [details] [diff] [review]
9173071: 9172759: Fix crash on BASE64 encoded utf-16 text with invalid charset.

[Triage Comment]
Approved for esr78

(I debated whether to take this on esr given the beta isn't out too long - I was swayed by the fact that it has tests)

Comment 24

[Rob Lemley [:rjl]]

Thunderbird 78.2.3:
https://hg.mozilla.org/releases/comm-esr78/rev/a4fe4108fb7c
https://hg.mozilla.org/releases/comm-esr78/rev/4cabf732dda2