crash in JS_NewStringCopyZ(JSContext*, char const*) via mime_write_message_body. compose with encrypted attachments?

VERIFIED FIXED in Thunderbird 53.0

Status

MailNews Core
MIME
--
critical
VERIFIED FIXED
3 years ago
3 months ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+2))

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
Thunderbird 53.0
x86
Windows NT
crash, dataloss, regression, regressionwindow-wanted, topcrash-thunderbird

Thunderbird Tracking Flags

(thunderbird_esr4551+ fixed, thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed)

Details

(Whiteboard: [regression:TB31?], crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

#12 crash for TB31.1.1 
and common in TB34.0a1

all start with frame of mime_write_message_body

This bug was filed from the Socorro interface and is 
report bp-5ff6fcdb-afa3-4e00-8b37-9e8b12140916.
=============================================================
0 	mozjs.dll 	JS_NewStringCopyZ(JSContext*, char const*) 	js/src/jsapi.cpp
1 	xul.dll 	XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) 	js/xpconnect/src/XPCConvert.cpp
2 	xul.dll 	nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	js/xpconnect/src/XPCWrappedJSClass.cpp
3 	xul.dll 	nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	js/xpconnect/src/XPCWrappedJS.cpp
4 	xul.dll 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp
5 	xul.dll 	SharedStub 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp
6 	xul.dll 	mime_write_message_body(nsIMsgSend*, char const*, unsigned int) 	mailnews/compose/src/nsMsgSend.cpp

bp-b3886295-e04d-443b-be99-835f42140825
Perhps the same crash is TB35.0a2 signature  JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) 
as in https://crash-stats.mozilla.com/report/index/d77090ce-42b3-4e55-b18e-eeda92141129 ... but I didnot compare source line numbers
Crash Signature: [@ JS_NewStringCopyZ(JSContext*, char const*)] → [@ JS_NewStringCopyZ(JSContext*, char const*)] [@ JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) ]

Comment 2

2 years ago
Just ran into this crash, too, and all i did was try to send an e-mail for which I had just turned *off* S/MIME encryption before sending.

https://crash-stats.mozilla.com/report/index/4c863b95-6fa4-4dff-a50d-168292150216

Comment 3

2 years ago
This crash doesn't depend on the message (e-mail) IMHO. I send e-mails to spam reporting server very often and spam messages are sent as mail attachments. Sometimes (let's say for ca 2% sendings) the crash happens. But after Thunderbird restarts the same spam is sent correctly. So it doesn't depend on what is being sent. I also observed the crash while sending the first message after Thunderbird restart. Looks like some undefined variable randomly plays the game here.
Still topcrash in version 31.6.0. All crashes I check have enigmail installed.
cc: Patrick in case he has new ideas

Too bad we don't have more than 7 months of crash data. I can only get back as far as 31.0 - examples: bp-e60e03fa-5017-4083-be67-bd3282141009 bp-d00f2a9a-1fdb-42ee-be72-8ac592141006.  We do however have ludo's bug 990594 reported against 31.0a1 buildid 20140329030204 bp-3660fe06-ce9a-4859-a297-6b19c2140401.
http://hg.mozilla.org/releases/comm-beta/annotate/aaeb4113ff88/mailnews/compose/src/nsMsgSend.cpp#l1225
bugzilla@1692 1219  if (!output)
hg@0 	      1220    return NS_MSG_ERROR_WRITING_FILE;
hg@0 	      1221
hg@0 	      1222  state->GetCryptoclosure(getter_AddRefs(crypto_closure));
hg@0 	      1223  if (crypto_closure)
hg@0 	      1224  {
hg@0 	      1225    return crypto_closure->MimeCryptoWriteBlock (buf, size);

Crash was common in version 34 beta. But in 2015 for all non-release channels we only have these:
bp-d77090ce-42b3-4e55-b18e-eeda92141129 TB35.0a2
bp-4ef2d64e-933c-4c9a-b3ab-2d6902150311 TB36.0b1
bp-b11a01a8-dead-4332-ae06-93e602150330 TB37.0b1 Build ID 20150311003753 

So perhaps it is gone in 38. We await proof via affected users testing version 38. :)
http://hg.mozilla.org/comm-central/filelog/1e8d5c907a77/mailnews/compose/src/nsMsgSend.cpp
Keywords: dataloss
See Also: → bug 990594
Summary: crash in JS_NewStringCopyZ(JSContext*, char const*) via mime_write_message_body. compose with attachments? → crash in JS_NewStringCopyZ(JSContext*, char const*) via mime_write_message_body. compose with encrypted attachments?
Whiteboard: [regression:TB??] → [regression:TB31?]
bp-31e9bdfa-73d3-4860-9291-115132150318 "GnuPG (via Enigmail) dialog came up to unlock signing key; not sure if related to Enigmail or just the attempt to send (or to nothing at all). "
bp-bceecf91-39b6-438d-af02-de6432150405 " Just entered password for PGP key to sign mail in Enigmail"

Comment 6

2 years ago
There is an important difference here between Thunderbird with and without Enigmail.

Without Enigmail, the method called is this (i.e. C++):
http://mxr.mozilla.org/comm-beta/source/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#954

With Enigmail installed, the method called is this (i.e. JavaScript):
https://sourceforge.net/p/enigmail/source/ci/master/tree/package/mimeEncrypt.js#l294
(Reporter)

Updated

2 years ago
Blocks: 990594
See Also: bug 990594
In version 38.1.0 this appears to be JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<T>, void const*, nsXPTType const&, nsID const*, nsresult*) 
currently ranged #50

bp-dabf0856-2b14-484e-bf49-d73312150809
 0 	xul.dll	JS_NewStringCopyZ(JSContext*, char const*)	js/src/jsapi.cpp
1 	xul.dll	XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, nsresult*)	js/xpconnect/src/XPCConvert.cpp
2 	xul.dll	nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*)	js/xpconnect/src/XPCWrappedJSClass.cpp
3 	xul.dll	nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*)	js/xpconnect/src/XPCWrappedJS.cpp
4 	xul.dll	PrepareAndDispatch	xpcom/reflect/xptcall/md/win32/xptcstubs.cpp
5 	xul.dll	SharedStub	xpcom/reflect/xptcall/md/win32/xptcstubs.cpp
6 	xul.dll	mime_write_message_body(nsIMsgSend*, char const*, unsigned int)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/compose/src/nsMsgSend.cpp:1203
7 	xul.dll	nsMsgSendPart::PushBody(char const*, int)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/compose/src/nsMsgSendPart.cpp:287
8 	xul.dll	nsMsgSendPart::Write()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/compose/src/nsMsgSendPart.cpp:608
9 	xul.dll	nsMsgComposeAndSend::GatherMimeAttachments()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/compose/src/nsMsgSend.cpp:909
10 		@0x1865cfff
Crash Signature: [@ JS_NewStringCopyZ(JSContext*, char const*)] [@ JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) ] → [@ JS_NewStringCopyZ(JSContext*, char const*)] [@ JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) ] [@ JS_NewStringCopyZ(JSContext*,&hellip;
Keywords: topcrash-thunderbird
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Updated

2 years ago
Crash Signature: [@ JS_NewStringCopyZ(JSContext*, char const*)] [@ JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) ] [@ JS_NewStringCopyZ(JSContext*,&hellip; → [@ JS_NewStringCopyZ(JSContext*, char const*)] [@ JS_NewStringCopyZ(JSContext*, char const*) | XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, tag_nsresult*) ] [@ JS_NewStringCopyZ(JSContext*,&hellip;

Comment 9

a year ago
https://crash-stats.mozilla.com/report/index/3478eca9-daa9-42f9-af35-425a02151229 

Same problem, Enigmail is running. Thunderbird crash when sending the mail !
(In reply to services from comment #9)
> https://crash-stats.mozilla.com/report/index/3478eca9-daa9-42f9-af35-
> 425a02151229 
> 
> Same problem, Enigmail is running. Thunderbird crash when sending the mail !

@services, can you reproduce?
Flags: needinfo?(services)
Keywords: topcrash-thunderbird
(In reply to Patrick Brunschwig from comment #6)
> There is an important difference here between Thunderbird with and without
> Enigmail.
> 
> Without Enigmail, the method called is this (i.e. C++):
> http://mxr.mozilla.org/comm-beta/source/mailnews/extensions/smime/src/
> nsMsgComposeSecure.cpp#954

Thanks for the info.  I don't see ScopedCERTCertificat in the stacks of bp-31e9bdfa-73d3-4860-9291-115132150318 and bp-bceecf91-39b6-438d-af02-de6432150405.  

> With Enigmail installed, the method called is this (i.e. JavaScript):
> https://sourceforge.net/p/enigmail/source/ci/master/tree/package/mimeEncrypt.
> js#l294

What should we see in the stack for this case?


Ignoring all the other signatures for the moment...

100% of JS_NewStringCopyZ | XPCConvert::NativeData2JS has enigmail
https://crash-analysis.mozilla.com/crash_analysis/20160306/20160306_Thunderbird_38.6.0-interesting-addons-with-versions.txt
eg bp-166decf1-bd73-49cd-a74f-2e5d92160306
Flags: needinfo?(patrick)
the reporter of bp-166decf1-bd73-49cd-a74f-2e5d92160306 claims it never crashes when using the send button. only happens when using ctrl+enter

Comment 13

a year ago
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #10)
> (In reply to services from comment #9)
> > https://crash-stats.mozilla.com/report/index/3478eca9-daa9-42f9-af35-
> > 425a02151229 
> > 
> > Same problem, Enigmail is running. Thunderbird crash when sending the mail !
> 
> @services, can you reproduce?

Can't reproduce. It happens sometimes. Doesn't crash when I don't cypher DRAFTS.
Flags: needinfo?(services)

Comment 14

a year ago
Hum It crashed this morning sorry... The mail had a picture in it...
(Reporter)

Comment 15

11 months ago
(In reply to services from comment #13)
> (In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment
> #10)
> > (In reply to services from comment #9)
> > > https://crash-stats.mozilla.com/report/index/3478eca9-daa9-42f9-af35-
> > > 425a02151229 
> > > 
> > > Same problem, Enigmail is running. Thunderbird crash when sending the mail !
> > 
> > @services, can you reproduce?
> 
> Can't reproduce. It happens sometimes. Doesn't crash when I don't cypher
> DRAFTS.

Are you saying it only happens if you enable auto-saving of drafts??
Flags: needinfo?(services)
(Reporter)

Updated

11 months ago
Duplicate of this bug: 1274970

Comment 17

11 months ago
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #15)
> (In reply to services from comment #13)
> > (In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment
> > #10)
> > > (In reply to services from comment #9)
> > > > https://crash-stats.mozilla.com/report/index/3478eca9-daa9-42f9-af35-
> > > > 425a02151229 
> > > > 
> > > > Same problem, Enigmail is running. Thunderbird crash when sending the mail !
> > > 
> > > @services, can you reproduce?
> > 
> > Can't reproduce. It happens sometimes. Doesn't crash when I don't cypher
> > DRAFTS.
> 
> Are you saying it only happens if you enable auto-saving of drafts? 


No, after some times the problem occurs even if autosaving draft is enabled.
(Reporter)

Updated

11 months ago
Duplicate of this bug: 1278475

Comment 19

9 months ago
Today I hit the same crash, first TB crash in 4y :-)

https://crash-stats.mozilla.com/report/index/772f76a4-ee8c-4fad-a15a-b32642160812

It happens after Enigmail prompted me for the password.

This is repeatable for me:

https://crash-stats.mozilla.com/report/index/eed4fbdf-273c-419b-90bf-bd52d2160812


What I did:
1) I have an IMAP account.

2) In that IMAP account I select a message I want to reply to and click the "Reply" button.

3) I write my reply and attach a *.log file per Drag&Drop.

4) I now click the Enigma buttons in the toolbar of the message window to enable encryption (lock) and signing (pen) for that message.

5) Now I press "Save" to make sure my draft is saved (lost my first mail due to that problem). I do not save my drafts encrypted.

6) Now I click the "Send" button.

7) Enigmail is prompts me like:

> Send  PGP/MIME SIGNED ENCRYPTED message to <recipient-address>?
> 
> Note: The message is encrypted for the following User ID's / Keys: 0x123456789

8) After clicking "Send message" Enigmail I am seeing the pinentry window asking me for my passphrase. Thunderbird is displaying "Creating mail" in the background. 

9) After entering my passphrase and HITTING [ENTER] key, Thunderbird crashed.


I am emphasizing that I hit the return key in the passphrase dialog because on my 3rd attempt I just clicked the button in the passphrase dialog and the mail was sent, no crash.

Comment 20

9 months ago
(In reply to Igor from comment #19)
> Today I hit the same crash, first TB crash in 4y :-)
> 
> https://crash-stats.mozilla.com/report/index/772f76a4-ee8c-4fad-a15a-
> b32642160812
> 
> It happens after Enigmail prompted me for the password.
> 
> This is repeatable for me:
> 
> https://crash-stats.mozilla.com/report/index/eed4fbdf-273c-419b-90bf-
> bd52d2160812
> 
> 
> What I did:
> 1) I have an IMAP account.
> 
> 2) In that IMAP account I select a message I want to reply to and click the
> "Reply" button.
> 
> 3) I write my reply and attach a *.log file per Drag&Drop.
> 
> 4) I now click the Enigma buttons in the toolbar of the message window to
> enable encryption (lock) and signing (pen) for that message.
> 
> 5) Now I press "Save" to make sure my draft is saved (lost my first mail due
> to that problem). I do not save my drafts encrypted.
> 
> 6) Now I click the "Send" button.
> 
> 7) Enigmail is prompts me like:
> 
> > Send  PGP/MIME SIGNED ENCRYPTED message to <recipient-address>?
> > 
> > Note: The message is encrypted for the following User ID's / Keys: 0x123456789
> 
> 8) After clicking "Send message" Enigmail I am seeing the pinentry window
> asking me for my passphrase. Thunderbird is displaying "Creating mail" in
> the background. 
> 
> 9) After entering my passphrase and HITTING [ENTER] key, Thunderbird crashed.
> 
> 
> I am emphasizing that I hit the return key in the passphrase dialog because
> on my 3rd attempt I just clicked the button in the passphrase dialog and the
> mail was sent, no crash.

Damn... here TB has already crashed several times since I installed Enigmail.

Do you mean you are able to reproduce it?

I have tried all I could to reproduce it but here it always happens randomly.

I have added R Kent to Cc.

I hope it will now be possible to fix it.

Comment 21

9 months ago
I did not trace through all of the code, but looking at the IDL for the call:

void mimeCryptoWriteBlock(in string aBuf, in long aLen)

and the actual call:

return crypto_closure->MimeCryptoWriteBlock (buf, size);

it looks to me like the call should have been marked [noscript] as defined, because I assume that the buf is not assumed to be null terminated. IIUC, JS will attempt to convert the buf to a string assuming it is null terminated. If no nulls are actually encountered, Bad Things can happen.

If this is the case, one possible fix would be to copy buf into a null terminated string at this point.

One possible fix, if my interpretation is correct,
(Assignee)

Comment 22

9 months ago
Looking at the other uses of MimeCryptoWriteBlock
https://dxr.mozilla.org/comm-central/search?q=imeCryptoWriteBlock&redirect=false
some provide a null terminated string, others don't, for example:
https://dxr.mozilla.org/comm-central/rev/be4c5a82c1c04686dedf282cc9fca2168c361b8b/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#1199

So no crashes in nsMsgComposeSecure.cpp, line 1199?
Duplicate of this bug: 1307985
(Assignee)

Comment 24

4 months ago
What are we going to do with this bug? Do people still crash?

Should we make this [noscript] here:
https://dxr.mozilla.org/comm-central/rev/e69ced0974d588e438569c22f71a0fcbf84c2b8e/mailnews/compose/public/nsIMsgComposeSecure.idl#24

Patrick, can you give us an update, please. Also, the links from comment #6 are out-of-date.

Comment 25

4 months ago
(sorry, somehow bugzilla doesn't send me any mails anymore, event though my prefs say that I want mails...)

People don't report crashes to me, and I never experienced crashes either, so I can't really comment on the crash situation.

However, by making mimeCryptoWriteBlock [noscript] you make it impossible for Enigmail to create PGP/MIME messages, thus that's a no-go for me.
Flags: needinfo?(patrick)
(Reporter)

Comment 26

4 months ago
Igor, stefan,  does it still reproduce for you?

Crash-stats indicates over 500 Windows users of current releases are affected in the past week.
Flags: needinfo?(stefan.hoelzle)
Flags: needinfo?(services)
Flags: needinfo?(igor.sverkos)
See Also: → bug 1281553
(Assignee)

Comment 27

4 months ago
OK, we need to go with what Kent said in comment #21:
> one possible fix would be to copy buf into a null terminated string at this point.

Comment 28

4 months ago
TB just crashed moments ago when I inserted my key :(

Luckily I had saved the message before I pressed SEND.
(Assignee)

Comment 29

4 months ago
I'll submit a patch in the next few days, this situation is really untenable.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Assignee)

Comment 30

4 months ago
Created attachment 8818958 [details] [diff] [review]
1069275-enigmail-crash.patch (v1)

I know you don't like crashing the app, but it would crash anyway ;-)
Flags: needinfo?(stefan.hoelzle)
Flags: needinfo?(igor.sverkos)
Attachment #8818958 - Flags: review?(rkent)
(Assignee)

Comment 31

4 months ago
OK, here comes a try build, especially for Marco so he can see whether this fixes his crashes:

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3bcc444495a6fc641890caa6d67b8a48c83200a

Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-d3bcc444495a6fc641890caa6d67b8a48c83200a/
(Assignee)

Comment 32

4 months ago
Comment on attachment 8818958 [details] [diff] [review]
1069275-enigmail-crash.patch (v1)

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1237,5 @@
>    if (crypto_closure)
>    {
> +    // We know this used to crash in the JS glue if MimeCryptoWriteBlock() was
> +    // implemented in JS. So sadly we need to copy to a new buffer to be able
> +    // to null-terminate.

We could test buf[size] != 0 before copying everything. The is for the message body, so that's a lot to copy.

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +1201,5 @@
>  {
> +  // Better to crash here in a defined fashion than to crash in the JS glue
> +  // if MimeCryptoWriteBlock() is implemented in JS. So far no crashes are known.
> +  if (buf[size] != 0)
> +    NS_RUNTIMEABORT("Buffer not null-terminated");

Instead of crashing, we could copy here like in nsMsgSend.cpp.

Anyway, the test-rabbits can use the try build and report back ;-)

Comment 33

4 months ago
Created attachment 8819736 [details] [diff] [review]
version 2 (using nsCString)

I really prefer to use strings over malloc and free. I believe that you can create a standard string from a non-null terminated buffer, like this. If you agree with this approach, use this code and give my r+ with your assign.
Attachment #8819736 - Flags: review?(jorgk)
(Assignee)

Comment 34

4 months ago
Thanks for looking at this.

Yes, your version is nice but you haven't answered my questions. I checked the call sites of MimeCryptoWriteBlock() and found two where null-termination is not guaranteed. Only one reports a crash. So my/your patch does this:

Null-terminated at the faulty call-site and NS_RUNTIMEABORT at the other. I understand that you don't like aborts.

I'd like to know how you assess the alternative:
At both call sites check for null-termination first, if already null-terminated, do nothing, otherwise, copy the string.

What do you think?
Flags: needinfo?(rkent)

Comment 35

4 months ago
I took your patch as the minimum that we need to do, and we could do that if you want with my suggested changes.

But I suppose you are asking me what we should really do.

The goal is that nsIMsgComposeSecure be scriptable, which means that the current description of mimeCryptoWriteBlock (which allows non-terminated strings) is incorrect and should be fixed. So I would use a different, renamed method (like mimeCryptoWriteString) without the length parameter, and change all internal uses to use that. Enigmail will need to make the minor change to rename and change the number of passed parameter. Internal uses can use the string classes as I suggested.
Flags: needinfo?(rkent)
(Assignee)

Comment 36

4 months ago
Somehow we're not coming together here ;-(

I've thought about introducing a new method, but we'd still need to copy a potentially long message body to call it, so apart from being nicer, that doesn't buy us any better performance.

My question was this: How about changing your proposal to

  if (crypto_closure)
  {
    // Already null-terminated?
    if (buf[size] == 0)
      return crypto_closure->MimeCryptoWriteBlock (buf, size);

    // We know this used to crash in the JS glue if MimeCryptoWriteBlock() was
    // implemented in JS. So sadly we need to copy to a new buffer to be able
    // to null-terminate.
    nsCString bufWithNull;
    bufWithNull.Assign(buf, size);
    return crypto_closure->MimeCryptoWriteBlock(bufWithNull.get(), size);
  }

And the question is also whether to treat the other call site the same instead of using NS_RUNTIMEABORT.
Flags: needinfo?(rkent)
(Reporter)

Updated

4 months ago
Duplicate of this bug: 1324762
(Assignee)

Comment 38

4 months ago
Created attachment 8820418 [details] [diff] [review]
1069275-enigmail-crash.patch (v3),  using nsCString and with test for null byte

This is what I meant in comment #36.

Now, strictly speaking I shouldn't access buf[size] if 'buf' is only allocated with size 'size'. We'd be extremely unlucky for this to cause a crash.

Since you didn't complain about the
+  if (buf[size] != 0)
+    NS_RUNTIMEABORT("Buffer not null-terminated");
I guess you share my view.
Attachment #8818958 - Attachment is obsolete: true
Attachment #8818958 - Flags: review?(rkent)
(Assignee)

Updated

4 months ago
Attachment #8820418 - Flags: review?(rkent)

Comment 39

4 months ago
Comment on attachment 8820418 [details] [diff] [review]
1069275-enigmail-crash.patch (v3),  using nsCString and with test for null byte

OK, we are close.

I just want two things, and I apologize if I am being inconsistent.

First, I don't see the value of all of this:

+    // Check for null-termination, if not found, copy to new string so JS glue
+    // doesn't crash when MimeCryptoWriteBlock() is implemented in JS.
+    if (buf[size] == 0)
+      return crypto_closure->MimeCryptoWriteBlock (buf, size);

We have no reason to believe that buf is null terminated, so I would expect this would do nothing the vast majority of the time, and in doing the check you introduce the buf[size] risk that you mentioned. Just always null-terminate. This code is mostly used in Enigmail anyway, so it does not cause a performance hit in more typical operation.

Second, at least file a bug to fix MimeCryptoWriteBlock to remove the unneeded size attribute, or mark it as unscripted and replace with a scriptable version.
Flags: needinfo?(rkent)
Attachment #8820418 - Flags: review?(rkent) → review-
(Assignee)

Comment 40

4 months ago
Created attachment 8820859 [details] [diff] [review]
1069275-enigmail-crash.patch (v4), using nsCString

Hmm, so I removed the buf[size] test from *your* version 2 and we now null-terminate in both call sites.

I'll file a bug for the follow-up work.
Attachment #8820859 - Flags: review?(rkent)
(Assignee)

Updated

4 months ago
Attachment #8819736 - Attachment is obsolete: true
Attachment #8819736 - Flags: review?(jorgk)
(Assignee)

Updated

4 months ago
Attachment #8820418 - Attachment is obsolete: true

Updated

4 months ago
Attachment #8820859 - Flags: review?(rkent) → review+
(Assignee)

Comment 41

4 months ago
https://hg.mozilla.org/comm-central/rev/9a56d29f240d0b880799af2c023473fcc6f72fbb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-thunderbird51: --- → affected
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 42

4 months ago
Comment on attachment 8820859 [details] [diff] [review]
1069275-enigmail-crash.patch (v4), using nsCString

[Approval Request Comment]
Regression caused by (bug #): Seems to always have been wrong.
User impact if declined: TB crashes when sending and using Enigmail.
Testing completed (on c-c, etc.): To be honest, we didn't test it at all.
Risk to taking this patch (and alternatives if risky):
Low, simple change to create and pass a null-terminated string.
Attachment #8820859 - Flags: approval-comm-esr45?
Attachment #8820859 - Flags: approval-comm-beta+
Attachment #8820859 - Flags: approval-comm-aurora+
(Assignee)

Comment 43

4 months ago
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/81519eb23a8267d414d9d025f3399fb3dc4fc681
status-thunderbird52: affected → fixed
(Assignee)

Comment 44

4 months ago
Beta (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/b970baae33f89fd32eb9a6cd807fd9a7c2d1c9cf
status-thunderbird51: affected → fixed
(Reporter)

Comment 45

3 months ago
I'd say this is ready for 45.x uplift. (#20 crash for 45.6.0)

Signature occurs 1-2x per day for 50beta. After several days there are no 51 beta crashes.
https://crash-stats.mozilla.com/search/?signature=%3DJS_NewStringCopyZ%20%7C%20XPCConvert%3A%3ANativeData2JS&release_channel=%21release&product=Thunderbird&date=%3E%3D2016-12-18T11%3A11%3A05.000Z&date=%3C2017-01-18T11%3A11%3A05.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports
(Reporter)

Comment 46

3 months ago
Further comfirmation, 52.0a2 had 1-2 crashes per week, but the last 52.0a2 crash is bp-1d808abe-d00f-40b4-b8b0-d3b802161214 20161212004003 
Definitely worthy of uplift.
(Assignee)

Comment 47

3 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #46)
> Definitely worthy of uplift ...
... to TB 45.x. It's already in the 5x.y series ;-)

Comment 48

3 months ago
Comment on attachment 8820859 [details] [diff] [review]
1069275-enigmail-crash.patch (v4), using nsCString

https://hg.mozilla.org/releases/comm-esr45/rev/1fcfc79ad515
Attachment #8820859 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

3 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 51+
(Reporter)

Comment 49

3 months ago
gone in v45.7.0 and 51 beta
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.