1197012 - crash in nsAString_internal::Assign(nsAString_internal const&, mozilla::fallible_t const&) | nsAString_internal::Assign(nsAString_internal const&) | mozilla::ErrorResult::ThrowErrorWithMessage(__va_list_tag*, mozilla::dom::ErrNum, nsresult)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Harald Kirschner :digitarald]

This bug was filed from the Socorro interface and is 
report bp-123cedd1-aed8-4e77-86b9-9bfc82150820.
=============================================================

Fresh profile, opened https://johnme-gcm.appspot.com/chat/

- Signed in
- Closed tab
- Received a push notification
- Opened tab
- Refresh
- Waited
- Un-focus the window to do something else
- Crash

Comment 1

[Cătălin Badea (:catalinb)]

nikhil, could you please have a look at this crash?

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I don't know what is the appropriate security group.

This should be rated sec-moderate I think. It is the same problem where ThrowTypeError does not validate the format string and arguments. In this case the error type was expecting arguments but wasn't passed any.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix ThrowTypeError

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8650863 [details] [diff] [review]
Fix ThrowTypeError

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

Fun. I'm trying to make my improved error throwing code catch this case, at least by safely crashing.

Comment 5

[Andrew McCreight [:mccr8]]

This case ends up failing in a somewhat different way: it is reading whatever is next on the stack as a string. That's probably a little worse, but off hand it doesn't seem very easy to exploit.

Comment 6

[Andrew McCreight [:mccr8]]

There's another instance of this problem in ServiceWorkerRegistrationMainThread::ShowNotification(), where it throws MSG_NO_ACTIVE_WORKER. (I found this with my patch to detect these kinds of errors at compile time.)

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix ThrowTypeError

I've changed that callsite as well.

Comment 8

[Andrew McCreight [:mccr8]]

Is this ready for review and landing, nsm? Thanks.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Yes

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8652396 [details] [diff] [review]
Fix ThrowTypeError

Approval Request Comment
[Feature/regressing bug #]: Bug 1114554
[User impact if declined]: Possible security bug
[Describe test coverage new/current, TreeHerder]: Can't really test this, was a type checking bug.
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5af8f2d7ee3e

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652396 [details] [diff] [review]
Fix ThrowTypeError

Taking it but next time, please restraint from using "None" as a risk. Every patch brings a risk. Even one-liner...

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Harald, can you verify that this fix addresses your original report?

Comment 14

[:Harald Kirschner :digitarald]

Fixed for me