825545 - crash in js_NewStringCopyN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tobias (:Tobbi) Markus]

Steps to reproduce:
1. Install the JavaScript Deobfuscator Jetpack from https://addons.mozilla.org/en-US/firefox/addon/javascript-deobfuscator/?src=ss

2. Open the Deobfuscator from Tools > Web Developer > JavaScript Deobfuscator.

3. Open a new tab with grooveshark.com

Notice that it hangs the browser, then crashes.

Crash reports:
https://crash-stats.mozilla.com/report/index/bp-86c19f39-3eb7-4fad-b3de-634c22121230

https://crash-stats.mozilla.com/report/index/bp-26072cc0-c668-486f-ad3a-02f802121230

Comment 1

[:Benjamin Peterson]

You also need https://addons.mozilla.org/en-US/firefox/addon/grooveshark-no-ads/ to reproduce.

Comment 2

[:Benjamin Peterson]

Attachment: make ScriptSource work if it is currently being compressed

The problem is that a ScriptSource whose sourc is being compressed can be reentered through the debugger's new script hook.

This patch makes ScriptSource's API behave correctly if it is used reentrantly.

Comment 3

[bhavana bajaj [:bajaj]]

(In reply to :Benjamin Peterson from comment #2)
> Created attachment 696670 [details] [diff] [review]
> make ScriptSource work if it is currently being compressed
> 
> The problem is that a ScriptSource whose sourc is being compressed can be
> reentered through the debugger's new script hook.
> 
> This patch makes ScriptSource's API behave correctly if it is used
> reentrantly.

Considering we are so close to out Fx18 release, this is likely to go unfixed at this point.We have gone to build with all our beta's at this time.

Are there any other major website's being impacted here ?

Is this a Fx18 regression or if earlier versions of firefox are affected as well ? Do we know what bug regressed this ? Will be helpful to know this info to understand the need of tracking for future versions as well.

Comment 4

[:Benjamin Peterson]

(In reply to bhavana bajaj [:bajaj] from comment #3)
> Is this a Fx18 regression or if earlier versions of firefox are affected as
> well ? Do we know what bug regressed this ? Will be helpful to know this
> info to understand the need of tracking for future versions as well.

This is basically like bug 795104. It only manifests itself when the javascript debugger is being used in a certain way. The STR here rely on having two different addons installed, though in theory, I don't see why it couldn't happen with different addons that use the debugger. It certainly isn't common.

17 is affect which is why I asked for esr tracking. Maybe it could be considered for a 18 point release if that happens.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to :Benjamin Peterson from comment #4)
> (In reply to bhavana bajaj [:bajaj] from comment #3)
> > Is this a Fx18 regression or if earlier versions of firefox are affected as
> > well ? Do we know what bug regressed this ? Will be helpful to know this
> > info to understand the need of tracking for future versions as well.
> 
> This is basically like bug 795104. It only manifests itself when the
> javascript debugger is being used in a certain way.

Given that, we won't track for upcoming releases.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 696670 [details] [diff] [review]
make ScriptSource work if it is currently being compressed

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

::: js/src/jsapi-tests/testSourcePolicy.cpp
@@ +29,5 @@
> +newScriptHook(JSContext *cx, const char *fn, unsigned lineno,
> +              JSScript *script, JSFunction *fun, void *data)
> +{
> +    // Just need this not to fail.
> +    JS_StringEqualsAscii(cx, script->sourceData(cx), simpleSource, (JSBool *)data);

Actually we do check that the match succeeds, so the comment could be removed.

Might as well set *data to false if this JS_StringEqualsAscii call fails.

@@ +36,5 @@
> +BEGIN_TEST(testScriptSourceReentrant)
> +{
> +    JS::CompileOptions opts(cx);
> +    JSBool match = false;
> +    JS_SetNewScriptHook(JS_GetRuntime(cx), newScriptHook, &match);

Instead of `JS_GetRuntime(cx)`, write `rt`.

Comment 7

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/038ff90d816b

Comment 8

[:Benjamin Peterson]

Also

https://hg.mozilla.org/integration/mozilla-inbound/rev/488af66ab025

Comment 9

[:Benjamin Peterson]

Fixed shell builds
https://hg.mozilla.org/integration/mozilla-inbound/rev/f735af7a57c3

Comment 10

[:Benjamin Peterson]

Comment on attachment 696670 [details] [diff] [review]
make ScriptSource work if it is currently being compressed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 761723
User impact if declined: Crashes with the specific set of addons mentioned in this bug. Possibly others, though others don't seem to have been reported.
Testing completed (on m-c, etc.): On m-i now
Risk to taking this patch (and alternatives if risky): Low-risk
String or UUID changes made by this patch: None

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/038ff90d816b
https://hg.mozilla.org/mozilla-central/rev/488af66ab025
https://hg.mozilla.org/mozilla-central/rev/f735af7a57c3

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 696670 [details] [diff] [review]
make ScriptSource work if it is currently being compressed

Approving for uplift to FF20. This still seems very edge casey, so please renominate for beta if we get user reports of this issue.

Comment 13

[:Benjamin Peterson]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9356989d9152

Comment 14

[:Benjamin Peterson]

Attachment: backport to b2g18

Comment 15

[:Benjamin Peterson]

Comment on attachment 708555 [details] [diff] [review]
backport to b2g18

[Approval Request Comment]
Needed for tef+ bug 836515.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

If this is intended to land on v1_0_0 (which it presumably is if bug 836515 is tef+), this needs to be tef+ as well.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 708555 [details] [diff] [review]
backport to b2g18

Clearing the approval since this is now tef+ and you may uplift when ready.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ced57d302db4
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/2eb213a75f35

Comment 19

[Simona Badau, Desktop QA]

Verified that Firefox 20 beta 1 does not crash when following the STR from the Description and from Comment 1. 

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:
Build ID: 20130220104816
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0

There are also no crash reports in Socorro related with the listed signatures on Firefox 20 beta 1.

Comment 20

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 21 beta 4. Also there are no crash reports in Socorro related with the listed signatures for Firefox 21 beta.

Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.8:
Build ID: 20130423212553
Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0