1545998 - Crash in [@ ConvertToUTF8 | MimeInlineText_convert_and_parse_line]

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Wayne Mery (:wsmwk)]

This bug is for crash report bp-2d40653e-13ca-4b0f-8253-1b93b0190324.
Earliest buildid is 20190322095201 (2019-03-22)

Top 10 frames of crashing thread:

0 xul.dll ConvertToUTF8 comm/mailnews/mime/src/mimemoz2.cpp
1 xul.dll static int MimeInlineText_convert_and_parse_line comm/mailnews/mime/src/mimetext.cpp:362
2 xul.dll static int MimeInlineText_open_dam comm/mailnews/mime/src/mimetext.cpp:415
3 xul.dll static int MimeInlineText_parse_eof comm/mailnews/mime/src/mimetext.cpp:236
4 xul.dll static int MimeInlineTextHTML_parse_eof comm/mailnews/mime/src/mimethtm.cpp:177
5 xul.dll static int MimeMultipart_close_child comm/mailnews/mime/src/mimemult.cpp:550
6 xul.dll static int MimeMultipart_parse_line comm/mailnews/mime/src/mimemult.cpp:153
7 xul.dll mime_LineBuffer comm/mailnews/mime/src/mimebuf.cpp:238
8 xul.dll static int MimeObject_parse_buffer comm/mailnews/mime/src/mimeobj.cpp:240
9 xul.dll mime_LineBuffer comm/mailnews/mime/src/mimebuf.cpp:238

Comment 1

[Jorg K (CEST = GMT+2)]

The crash reason is pretty clearly stated:
MOZ_RELEASE_ASSERT(this->mData[substring_type::mLength] == 0) (nsTDependentString must wrap only null-terminated strings. You are probably looking for nsTDependentSubstring.)

That was introduced here:
https://searchfox.org/mozilla-central/diff/bdf43f53c2230a96da314c381db7056c4be6bc7a/xpcom/string/nsTString.h#453
Bug 1536689, landed 20th Match, two days before the crashes started.

Patch coming.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1545998-fix-crash.patch

First reviewer wins ;-)

Comment 3

[Jorg K (CEST = GMT+2)]

More interesting than the crash is the fact that this is on the UTF-7 path so someone must the reading old UTF-7 mail which was converted from Windows Live Mail (or Outlook Express?).

Comment 4

[:aceman]

Comment on attachment 9059736 [details] [diff] [review]
1545998-fix-crash.patch

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

OK, can you please see if a similar pattern at https://searchfox.org/comm-central/source/mailnews/base/util/nsMsgI18N.cpp#95 needs any treatment?

Comment 5

[Jorg K (CEST = GMT+2)]

That's not the pattern. These are the call sites that need looking at:
nsDependentCString.(.,[ a-z0-9_]+len
So nsDependentCString(some_buffer, some_length).

That search finds:
mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
576 nsDependentCString word (aWord, wordLength); // CHEAP, no allocation occurs here...
mailnews/mime/src/mimemoz2.cpp
825 rv = CopyUTF7toUTF16(nsDependentCString(stringToUse, inLength), utf16);
mailnews/mime/src/mimetpfl.cpp
392 CopyUTF8toUTF16(nsDependentCString(line, length), lineResult);
mailnews/mime/src/mimetpla.cpp
307 nsDependentCString inputStr(line, length);

One is fixed here. The one in nsBayesianFilter.cpp has a null-terminated aWord. That leaves mimetpfl.cpp and mimetpla.cpp. In mimetpla.cpp that call is in MimeInlineTextPlain_parse_line and that seems to be called with a null-terminated line. In mimetpfl.cpp that call is in MimeInlineTextPlainFlowed_parse_line and again, that should be OK.

Besides, we'd have very high crash rates since this code runs for every plaintext of flowed plaintext message displayed. So to the best of my knowledge, the patch fixes the only case.

Comment 6

[:aceman]

I meant the utf7 conversions at the 2 places, not 'DependentCString' itself, whether they do not need to be aligned.

Comment 7

[Jorg K (CEST = GMT+2)]

Nothing wrong with the UTF-7 conversion in nsMsgI18N.cpp. The problem is in the string handling to generate a "smart" string from a "raw" string.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/57dccd17e67f
Use nsDependentCSubstring() to avoid MOZ_RELEASE_ASSERT() introduced in bug 1536689. r=aceman