1510028 - TB crashes editing a .EML file without a 'Content-Type:' header @ nsTDependentString<T>::nsTDependentString<T> via CreateCompositionFields - mime_parse_stream_complete

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[timrude]

TB 60.3.0 and 60.3.1 on Windows 7

To reproduce:

Create a .eml file using another app (or edit a .eml file saved by TB) and omit/remove the 'Content-Type:' header line.

Open the .eml file in TB and it opens correctly in View mode. But then press Ctrl-E to switch to Edit mode and TB crashes.

In pre-60 versions of TB, if the 'Content-Type:' header line was missing, the content type was assumed to be text/plain and both View mode and Edit mode worked with no problem.

Comment 1

[Wayne Mery (:wsmwk)]

Please post your crash ids in text form into this bug report
see https://support.mozilla.org/en-US/kb/mozilla-crash-reporter-tb#w_viewing-crash-reports

Comment 2

[timrude]

Crash ID: b62557a7-bead-48bb-8f6e-ad6f70181127

Comment 3

[Wayne Mery (:wsmwk)]

bp-b62557a7-bead-48bb-8f6e-ad6f70181127 is @ nsTDependentString<T>::nsTDependentString<T>

0 	xul.dll 	nsTDependentString<char>::nsTDependentString<char>(char const*) 	xpcom/string/nsTDependentString.h:80
1 	xul.dll 	CreateCompositionFields(char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char*, nsIMsgCompFields**) 	comm/mailnews/mime/src/mimedrft.cpp:352
2 	xul.dll 	mime_parse_stream_complete 	comm/mailnews/mime/src/mimedrft.cpp:1393
3 	xul.dll 	nsStreamConverter::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	comm/mailnews/mime/src/nsStreamConverter.cpp:1053
4 	xul.dll 	nsMsgProtocol::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	comm/mailnews/base/util/nsMsgProtocol.cpp:389
5 	xul.dll 	nsMailboxProtocol::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	comm/mailnews/local/src/nsMailboxProtocol.cpp:416
6 	xul.dll 	nsInputStreamPump::OnStateStop() 	netwerk/base/nsInputStreamPump.cpp:706

Based on source lines I suspect these other signatures are related 
* strlen | CreateCompositionFields  bp-66077973-621f-44cd-a7c4-246c80181124
* CreateCompositionFields  bp-cdb88f7b-41c6-44c9-a28e-7dc0c0181126

nsTDependentString<T>::nsTDependentString<T> is the most common signature but not all stacks for that signature relate to this bug.

All crashes are version 60, so let's assume regression

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to timrude from comment #0)
> Create a .eml file using another app (or edit a .eml file saved by TB) and
> omit/remove the 'Content-Type:' header line.
Right. Now you don't have a charset and will crash here:

  if (from) {
    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(from),
                                    nsDependentCString(charset),  <===
                                    outString);
    cFields->SetFrom(outString);
  }

Patch coming.

Comment 5

[Jorg K (CEST = GMT+2)]

BTW, the change is from here: https://hg.mozilla.org/comm-central/rev/bc4dff04bede#l17.62

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1510028-no-null-in-nsDependentCString.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9027859 [details] [diff] [review]
1510028-no-null-in-nsDependentCString.patch

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

LGTM, r=mkmelin

Comment 8

[Jorg K (CEST = GMT+2)]

I'm afraid that's not enough. I really need to go through https://hg.mozilla.org/comm-central/rev/bc4dff04bede and check all the call sites where nsDependentCString() was added. The argument may be null. Sigh.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1510028-no-null-in-nsDependentCString.patch (v2)

I went through bc4dff04bede with a fine comb looking for
  ^\+.*nsDependentCString

That gave 29 hits:
Line 94: +      nsMsgI18NConvertToUnicode(nsDependentCString(charset), OK
Line 238: +    if (NS_SUCCEEDED(nsMsgI18NConvertFromUnicode(nsDependentCString(charset), FIXED
Line 528: +    rv = nsMsgI18NConvertFromUnicode(nsDependentCString(m_compFields->GetCharacterSet()),
Line 635: +    rv = nsMsgI18NConvertFromUnicode(nsDependentCString(aCharset), OK
Line 658: +      rv = nsMsgI18NConvertFromUnicode(nsDependentCString(aCharset), OK
Line 702: +                                   nsDependentCString(m_attachment1_body), OK
Line 791: +    nsMsgI18NConvertToUnicode(nsDependentCString(fallbackCharset), FIXED
Line 815: +  nsMsgI18NConvertFromUnicode(nsDependentCString(msg.GetBodyCharset()), bodyW, bodyA); FIXED
Line 860: +                                              nsDependentCString((char *) aFiles[i].lpszFileName), OK
Line 883: +                                   nsDependentCString((char *) aMessage->lpszSubject), OK
Line 902: +                                   nsDependentCString((char *) aMessage->lpszNoteText), OK
Line 946: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(from), OK
Line 947: +                                    nsDependentCString(charset), FIXED
Line 959: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(reply_to), OK
Line 960: +                                    nsDependentCString(charset), FIXED
Line 967: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(to), OK 
Line 968: +                                    nsDependentCString(charset), FIXED
Line 975: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(cc), OK 
Line 976: +                                    nsDependentCString(charset), FIXED
Line 983: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(bcc), OK 
Line 984: +                                    nsDependentCString(charset), FIXED
Line 1004: +            rv = nsMsgI18NConvertToUnicode(nsDependentCString(bodyCharset), OK
Line 1005: +                                           nsDependentCString(body), OK
Line 1029: +                                   nsDependentCString(opt->default_charset), FIXED
Line 1049: +      if (NS_SUCCEEDED(nsMsgI18NConvertFromUnicode(nsDependentCString(charset), OK
Line 1094: +          rv = nsMsgI18NConvertToUnicode(nsDependentCString(mailCharset), OK
Line 1115: +      rv = nsMsgI18NConvertFromUnicode(nsDependentCString(mailCharset), OK
Line 1139: +        rv = nsMsgI18NConvertToUnicode(nsDependentCString(mailCharset), OK
Line 1161: +        rv = nsMsgI18NConvertFromUnicode(nsDependentCString(mailCharset), OK

which I checked individually. OK means it's clear from the context the the argument can't be null.

Five fixes were in the previous patch, here I'm adding four more to make it nine. Most likely those additional four are not necessary, but better be safe than sorry. Not worth a crash.

Comment 10

[Jorg K (CEST = GMT+2)]

Oops, miscounted that, five additional fixes, this one here
Line 528: +    rv = nsMsgI18NConvertFromUnicode(nsDependentCString(m_compFields->GetCharacterSet()),
is also a FIXED.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9027871 [details] [diff] [review]
1510028-no-null-in-nsDependentCString.patch (v2)

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

::: mailnews/mime/src/mimehdrs.cpp
@@ +41,5 @@
>      nsAutoCString output;
>      nsMsgI18NConvertRawBytesToUTF8(value,
> +                                   opt->default_charset
> +                                     ? nsDependentCString(opt->default_charset)
> +                                     : EmptyCString(),

odd ? placement, usually it's on the previous line

Comment 12

[Jorg K (CEST = GMT+2)]

Note that passing null to nsMsgI18NConvertFromUnicode() when it still had a char* argument would have crashed here:
https://hg.mozilla.org/comm-central/rev/bc4dff04bede#l5.24 or here
https://hg.mozilla.org/comm-central/rev/bc4dff04bede#l5.53

Before switching to encoding_rs the code was like this (line in TB 52):
https://dxr.mozilla.org/comm-esr52/source/mailnews/base/util/nsMsgI18N.cpp#119
and
https://dxr.mozilla.org/comm-esr52/source/mailnews/base/util/nsMsgI18N.cpp#128
would also have crashed on an empty charset on !*aCharset dereferencing null.

So I suspect, the crash in mimedrft.cpp is new, before it would have crashed in nsMsgI18N.cpp in nsMsgI18NConvertToUnicode() or nsMsgI18NConvertFromUnicode() when those received and empty charset.

Were they any crashes of that nature that have disappeared?

Surely doing those charset conversions with an empty charset will lead to "interesting" results ;-(

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #11)
> odd ? placement, usually it's on the previous line
Actually, no, I'm following our linting effort now where the ? goes to start of the line. Looking in MsgComposeCommands.js there are both types now, so I'm confused. I'll do some checking before landing this, but I'm pretty sure lately I've been doing them all like this.

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) from comment #12)
> ...
> So I suspect, the crash in mimedrft.cpp is new, before it would have crashed
> in nsMsgI18N.cpp in nsMsgI18NConvertToUnicode() or
> nsMsgI18NConvertFromUnicode() when those received and empty charset.
> 
> Were they any crashes of that nature that have disappeared?

We have several low volume crashes (that don't have bug report) that don't happen in version 60 from https://crash-stats.mozilla.com/search/?signature=~nsMsgI18NConvertToUnicode&product=Thunderbird&date=%3E%3D2018-08-27T09%3A22%3A44.000Z&date=%3C2018-11-27T08%3A22%3A44.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature 

OOM | large | NS_ABORT_OOM | AppendUTF8toUTF16 | CopyUTF8toUTF16 | nsMsgI18NConvertToUnicode   
OOM | large | NS_ABORT_OOM | AppendASCIItoUTF16 | CopyASCIItoUTF16 | nsMsgI18NConvertToUnicode   
shutdownhang | ConvertUTF8toUTF16::write | AppendUTF8toUTF16 | AppendUTF8toUTF16 | CopyUTF8toUTF16 | nsMsgI18NConvertToUnicode  
shutdownhang | memcpy | nsAString_internal::Assign | nsAString_internal::Assign | nsAString_internal::Assign | nsMsgI18NConvertToUnicode   
shutdownhang | nsMsgI18NConvertToUnicode   

And https://crash-stats.mozilla.com/signature/?product=Thunderbird&_sort=-date&signature=OOM%20%7C%20large%20%7C%20NS_ABORT_OOM%20%7C%20CopyUTF16toUTF8%20%7C%20nsMsgI18NConvertFromUnicode&date=%3E%3D2018-08-27T09%3A22%3A44.000Z&date=%3C2018-11-27T08%3A22%3A44.000Z

Comment 15

[Magnus Melin [:mkmelin]]

Dunno which linting efforts you refer to. That is an odd style. *All* the eslint examples place ? on the first line - https://eslint.org/docs/rules/multiline-ternary

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Wayne Mery (:wsmwk) from comment #14)
> We have several low volume crashes (that don't have bug report) that don't
> happen in version 60 from
... yes, in nsMsgI18NConvertToUnicode and nsMsgI18NConvertFromUnicode as predicted.

(In reply to Magnus Melin [:mkmelin] from comment #15)
> Dunno which linting efforts you refer to.
Linting in mail/ and mailnews/. Anyway, I was a friend of

x ?
  y :
  z

Doing this in Calendar, where I must have coded something, gives
2357:80  error  ':' should be placed at the beginning of the line.  operator-linebreak (eslint)

So I assumed our linting rules mandated ? and : a the front since surely you don't want to place ? at the end and : at the front.

Looking at the freshly linted MsgComposeCommands.js I see:
  let msgCompFormat = (aEvent && aEvent.shiftKey) ?
    Ci.nsIMsgCompFormat.OppositeOfDefault :
    Ci.nsIMsgCompFormat.Default;
and
      var recommAction = (convert == Ci.nsIMsgCompConvertible.No)
                          ? Ci.nsIMsgCompSendFormat.AskUser
                          : Ci.nsIMsgCompSendFormat.PlainText;

I also added some at the front here recently:
https://hg.mozilla.org/comm-central/rev/b0459a76e420#l3.25 (and two more in that changeset).

So how would you like it, I'm thoroughly confused now :-( - and wasted too much time of this nit-ty issue.

Comment 17

[Jorg K (CEST = GMT+2)]

We also had ? and : at the beginning of lines, I even took some out:
https://hg.mozilla.org/comm-central/rev/01215911a27e#l1.411
Looks for |   : | in the changeset.

I think

abcdefghijk
  ? xxx
  : yyy;

looks OK and so does

abcdefghijk ?
  xxx : yyy;

Comment 18

[Magnus Melin [:mkmelin]]

We definitely want operator-linebreak after, that's questionmarks at the end of line

So this is correct:

  let msgCompFormat = (aEvent && aEvent.shiftKey) ?
    Ci.nsIMsgCompFormat.OppositeOfDefault :
    Ci.nsIMsgCompFormat.Default;

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/096fefc1934f
Add null check to avoid crash due to nsDependentCString(null). r=mkmelin

Comment 20

[Jorg K (CEST = GMT+2)]

Magnus, I went with you style although mailnews/mime/src/mimehdrs.cpp is full of the ? and : at the front.

Comment 21

[timrude]

Thanks for the fix!

Comment 22

[Jorg K (CEST = GMT+2)]

TB 60.3.2 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/4f5ec4473e22a72d72241ef98216ce9fbe51c569

Comment 23

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/c839e782792f00b76b94fa6004745d9e2006321e

Comment 24

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/c839e782792f00b76b94fa6004745d9e2006321e