Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: gkw, Assigned: arai)

Tracking

(Blocks 2 bugs, {crash, regression, testcase})

Trunk
mozilla34
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox32 verified, firefox33 verified, firefox34 verified)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
Posted file stack
function f() {}
function g() {
    f(...Array(262145))
}
f()
g()
g()

crashes js opt shell on m-c changeset d7e78f0c1465 with --ion-eager at EnterBaseline.

My configure flags are:

MAKE=mozmake AR=ar sh c://Users//fuzz1win//trees//mozilla-central//js//src//configure --disable-debug --enable-optimize --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-nspr-build

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/bcf7062a8d78
user:        Tooru Fujisawa
date:        Fri May 16 01:50:14 2014 +0900
summary:     Bug 1003149 - Part2: Implement JSOP_SPREAD* optimized stubs in the baseline compiler. r=jandem

Jan, is bug 1003149 a likely regressor?

Setting s-s and assuming worse-case sec-critical as a start. This seems Win-only, but I'm not sure.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
arai would you be interested in investigating this? If you don't have time, no problem :)
Flags: needinfo?(jdemooij) → needinfo?(arai_a)
Assignee

Comment 3

5 years ago
Sure.

The problem is already reproduced for me too, with nightly jsshell-win32.zip on Windows 7,
and now I'm preparing build and debug environment on Windows (my Windows machine is very cheap, so it may take long time...).
Give me some time.
Flags: needinfo?(arai_a)
Ah I didn't notice it's Windows-only (at least this particular test). Setting up a Windows dev environment can take a while yes..

Looking at the testcase, we probably need to do a stack overflow check before pushing the arguments. The Baseline fun.apply stub only handles arrays with length up to MAX_ARGS_ARRAY_LENGTH (= 16), that's not a lot. We could probably do something similar here.
As mentioned in comment 4 and in the stack, this is a stack exhaustion problem (it dies on the push instruction), so this is not a security bug :) Opening up.
Group: core-security, javascript-core-security
Keywords: sec-critical
Assignee

Comment 6

5 years ago
Created draft patch.

(In reply to Jan de Mooij [:jandem] from comment #4)
> Looking at the testcase, we probably need to do a stack overflow check
> before pushing the arguments. The Baseline fun.apply stub only handles
> arrays with length up to MAX_ARGS_ARRAY_LENGTH (= 16), that's not a lot. We
> could probably do something similar here.

Thank you for letting me know that, I applied that change.
With this patch, optimized stubs (Scripted/Native) will be used only for spread call with less than 16 arguments, like fun.apply, and fallback stub will be used for others.

spread-call-near-maxarg.js does almost same test as spread-call-maxarg.js in the patch in bug 1003149,
but it passes ARGS_LENGTH_MAX-1 arguments, so it should not throw "too many function arguments", but "too much recursion" if happens.
I confirmed this test crashes on nightly 32bit jsshell on Windows 7.

Build and jstests/jit-test are passed on Mac OS X.
(the test does not crash on Mac OS X even without this patch though...)

My Windows machine is now building, but it will take half a day or more :(

By the way, can I push this patch and related tests to try server now?
Attachment #8473065 - Flags: feedback?(jdemooij)
Assignee

Comment 7

5 years ago
Confirmed that attachment #8473065 [details] [diff] [review] fixes the crash on Windows 7 32bit build, locally.

Compared with and applied to following m-c revision:
  http://hg.mozilla.org/mozilla-central/rev/5299864050ee

Without the patch:
  crash happened when running spread-call-near-maxarg.js with --ion-eager, at EnterBaseline

After applied the patch:
  no crash happened when running spread-call-near-maxarg.js with --ion-eager
Comment on attachment 8473065 [details] [diff] [review]
Do not use optimized stub for spread call with many arguments.

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

Looks good! Thanks for fixing it. r=me with nits addressed.

::: js/src/jit/BaselineIC.cpp
@@ +8523,5 @@
>  
> +    // Limit actual argc to something reasonable (huge number of arguments can
> +    // blow the stack limit).
> +    static_assert(ICCall_Scripted::MAX_ARGS_SPREAD_LENGTH <= ARGS_LENGTH_MAX,
> +                  "maximum arguments length for optimized stub should be less than or equal to ARGS_LENGTH_MAX");

Nit: this doesn't fit in 99 characters.

You can just split it up like this:

static_asser(a <= b,
             "aaaaaaaaaaa "
             "bbbbbbbbbbb");

Or use <= instead of "less than or equal to"

@@ +8524,5 @@
> +    // Limit actual argc to something reasonable (huge number of arguments can
> +    // blow the stack limit).
> +    static_assert(ICCall_Scripted::MAX_ARGS_SPREAD_LENGTH <= ARGS_LENGTH_MAX,
> +                  "maximum arguments length for optimized stub should be less than or equal to ARGS_LENGTH_MAX");
> +    masm.branch32(Assembler::Above, argcReg, Imm32(ICCall_Scripted::MAX_ARGS_SPREAD_LENGTH), failure);

Nit: same here (move "failure);" to its own line).

::: js/src/jit/BaselineIC.h
@@ +5606,5 @@
>  class ICCall_Scripted : public ICMonitoredStub
>  {
>      friend class ICStubSpace;
> +  public:
> +    // The maximum length of an inlineable spread call arguments.

Nit: s/length of an/number of/

@@ +5609,5 @@
> +  public:
> +    // The maximum length of an inlineable spread call arguments.
> +    // Keep this small to avoid controllable stack overflows by attackers passing large
> +    // arrays to spread call.
> +    // This value is shared with ICCall_Native.

Nit: reformat this comment to fit in 79 columns.
Attachment #8473065 - Flags: feedback?(jdemooij) → review+
Reporter

Updated

5 years ago
Crash Signature: [@ EnterBaseline]
Assignee

Comment 9

5 years ago
Updated comments and line wrapping.

Carrying forward jandem's r+ in comment #8

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=f986dbcf38ad
Attachment #8473065 - Attachment is obsolete: true
Attachment #8474078 - Flags: review+
Keywords: checkin-needed
Reporter

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0be3fb8a15
Assignee: nobody → arai_a
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla34
https://hg.mozilla.org/mozilla-central/rev/0f0be3fb8a15
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Assignee

Comment 12

5 years ago
Comment on attachment 8474078 [details] [diff] [review]
Do not use optimized stub for spread call with many arguments.

Approval Request Comment
[Feature/regressing bug #]: bug 1003149
[User impact if declined]: Crash due to stack overflow, by navigating to specially crafted page, which contains JavaScript in comment #0 and some loops to enable baseline execution for it. This is reproducible at least on Windows 32bit build.
[Describe test coverage new/current, TBPL]: Spread call with many arguments over than stack limit, but under than maximum argument length, run in baseline execution, with optimized stub.
[Risks and why]: Low. It disables optimized execution of spread call with many arguments (more than 16), which was introduced by patch Part 2 in bug 1003149, and fallback to more general code path, which was introduced in patch Part 1 in bug 1003149. So it decreases some performance improvement of bug 1003149.
[String/UUID change made/needed]: None
Attachment #8474078 - Flags: approval-mozilla-beta?
Attachment #8474078 - Flags: approval-mozilla-aurora?
Attachment #8474078 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Tooru Fujisawa [:arai] from comment #12)
> [Risks and why]: Low. It disables optimized execution of spread call with
> many arguments (more than 16), which was introduced by patch Part 2 in bug
> 1003149, and fallback to more general code path, which was introduced in
> patch Part 1 in bug 1003149. So it decreases some performance improvement of
> bug 1003149.

We're pretty close to the end of the beta cycle so I'd like to get a better understanding of the regression risk from this change. I see a green try run but is this case covered by automated tests? Has this configuration been well tested by the user base before bug 1003149 landed?
Flags: needinfo?(arai_a)
Assignee

Comment 15

5 years ago
Sorry I didn't have that perspective.

(In reply to Lawrence Mandel [:lmandel] from comment #14)
> We're pretty close to the end of the beta cycle so I'd like to get a better
> understanding of the regression risk from this change. I see a green try run
> but is this case covered by automated tests?

The case this change affects are tested by following file, it tests currectness of the behavior, and ensure it does not crash:
  js/src/jit-test/tests/basic/spread-call-near-maxarg.js

I'm afraid I'm not sure about performance test in automated tests.

> Has this configuration been
> well tested by the user base before bug 1003149 landed?

This configuration is not same as before bug 1003149, because this patch does not remove a effect of patch Part 1 in bug 1003149.

From performance aspect, it is still faster than before bug 1003149 (such as current mozilla-release), but will be slower than current 32 branch in some case.
If there are some code which relies on current performance on 32 branch and is performance critical, and it's not enough with reduced performance in this patch, I cannot say this change is safe (I guess that is very rare case though, because it will takes at most 1.4 times longer, for each spread call operation).


There are some new usage of spread call in 5 files in current mozilla-beta, from bug 1003149 to now.
The number of actual argument is less than 16 in almost cases (except last two cases, in csscoverage.js), so they should not be affected by this change, and run same as after bug 1003149.

browser/devtools/projecteditor/lib/editors.js
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/projecteditor/lib/editors.js#34
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/projecteditor/lib/editors.js#149
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/projecteditor/lib/editors.js#152

browser/devtools/projecteditor/lib/projecteditor.js
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/projecteditor/lib/projecteditor.js#488
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/projecteditor/lib/projecteditor.js#491
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/projecteditor/lib/projecteditor.js#513

browser/devtools/webide/modules/app-manager.js
  http://mxr.mozilla.org/mozilla-beta/source/browser/devtools/webide/modules/app-manager.js#78

toolkit/components/places/tests/unit/test_promiseBookmarksTree.js
  http://mxr.mozilla.org/mozilla-beta/source/toolkit/components/places/tests/unit/test_promiseBookmarksTree.js#77
  http://mxr.mozilla.org/mozilla-beta/source/toolkit/components/places/tests/unit/test_promiseBookmarksTree.js#126
  http://mxr.mozilla.org/mozilla-beta/source/toolkit/components/places/tests/unit/test_promiseBookmarksTree.js#145

toolkit/devtools/server/actors/csscoverage.js
  The number of actual arguments is the number of imported stylesheets from each CSS file, so sometimes greater than 16 (it's still rare case I guess, though).
  http://mxr.mozilla.org/mozilla-beta/source/toolkit/devtools/server/actors/csscoverage.js#485
  http://mxr.mozilla.org/mozilla-beta/source/toolkit/devtools/server/actors/csscoverage.js#503
Flags: needinfo?(arai_a)
Comment on attachment 8474078 [details] [diff] [review]
Do not use optimized stub for spread call with many arguments.

I reviewed this with aria. This change should only impact performance, which should still be improved over Firefox 31. I'm going to mark this as beta+. We will have two betas to review before the RC.

arai - Can you please confirm (with QA if they can help) that this patch fixes the crash once in a beta build?
Attachment #8474078 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(arai_a)
Assignee

Comment 18

5 years ago
Confirmed that the crash was fixed on latest beta build.
  Bad: Firefox 32.0 beta 7 on Windows 7 (32bit)
  Good: Firefox 32.0 beta 8 on Windows 7 (32bit)

I'll contact to someone in QA team for more check.
Then, can I post an example HTML file to test it easily? (which causes instant crash on Firefox 32.0 beta 7 or older)
Flags: needinfo?(arai_a)

Updated

5 years ago
Flags: qe-verify+
Assignee

Comment 19

5 years ago
Browser based testcase.
This could crash Firefox 32 beta7 or older version.
Reproduced the crash on Windows 7 32 and 64bit using old Nightly build (2014-08-14) and Firefox 32 beta 7 build, verified that latest Nightly, latest Aurora and Firefox 32 beta 8 does not crash using the testcase.
You need to log in before you can comment on or make changes to this bug.