Closed Bug 1001372 Opened 6 years ago Closed 6 years ago

[jsdbg2] Clone optimized ICStubs when doing debug mode OSR

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 --- wontfix
firefox-esr24 --- unaffected
firefox-esr31 - wontfix

People

(Reporter: decoder, Assigned: shu)

References

Details

(Keywords: crash, sec-low, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update])

Crash Data

Attachments

(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 5ecd532a167e (run with --fuzzing-safe):


var lfcode = new Array();
lfcode.push = loadFile;
lfcode.push("");
lfcode.push("");
lfcode.push("3");
lfcode.push("");
lfcode.push("");
lfcode.push("");
lfcode.push("");
lfcode.push("4");
lfcode.push("");
lfcode.push("0");
lfcode.push("gczeal(2);");
lfcode.push("\
var g = newGlobal();\
g.parent = this;\
g.eval('new Debugger(parent).onExceptionUnwind = f;');\
");
function loadFile(lfVarx) {
    evaluate(lfVarx);
}
Marking as a fuzzblocker, as this crashes on the heap with no reasonable signatures that could be used to distinguish it from other failures. Also it seems highly frequent.
Whiteboard: [jsbugmon:update,bisect][fuzzblocker]
Depends on: 716647
NI'ing shu based on bug 716647 comment 116.
Flags: needinfo?(shu)
Here's another test (requested by shu), that also crashes on the heap:


var lfcode = new Array();
lfcode.push("");
lfcode.push("0");
lfcode.push("newGlobal(\"1\").eval(\"(new Debugger).addAllGlobalsAsDebuggees();\");\n");
while (true) {
    var file = lfcode.shift(); if (file == undefined) { break; }
    loadFile(file)
}
function loadFile(lfVarx) {
    try {
      if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
        evaluate(lfVarx); 
      } else if (!isNaN(lfVarx)) {}
    } catch (lfVare) {    }
}
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140424014842" and the hash "25dd3d17a19a".
The "bad" changeset has the timestamp "20140424020245" and the hash "079f4f0ed6a6".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25dd3d17a19a&tochange=079f4f0ed6a6
This is caused by me completely missing handling of fake exit frames, as I thought Baseline didn't generate them.

The current debug mode OSR code depends on the exit frame always being a different ABI frame (i.e. pushed ip) from the stub frame. For fake exit frames generated by ICCall_Native, this is not true, so we are getting unpredictable crashes there. Closing bug since this is technically s-s, though a low rating because there is currently no frontend way to in the browser to attach a Debugger with scripts live on the stack -- and should be that way until the fuzzers calm down.

A fix here is going to be tricky. For normal callVMs, the wrapper persists outside of a particular BaselineScript, so VM calls always have a place to return to. It is the exit frame that the wrapper pushes that debug mode OSR patches. For ICCall_Native's optimized stubs, the fake exit frame is a per-BaselineScript jitcode, so if we destroy the original BaselineScript at debug mode OSR time, there will be no code for the VM call to return to.

I believe the cleanest way, at the cost of a little perf, is to have a real exit frame for ICCall_Native stubs. ICCall_Fallback would compile a little stub that expects a callee already loaded in a particular register (R1 for 32bit, ExtractTemp0 for 64bit), pushes a real exit frame, and does the callABI. When debug mode OSR happens, we then have persistent piece of jitcode to return to and a real exit frame to patch. This comes at the cost of an extra call, and an extra real ABI frame. My hope is that the perf hti from this will be negligible, since this is in Baseline.
Group: javascript-core-security
Flags: needinfo?(shu)
Summary: [jsdbg2] Crash on Heap near [@ js::jit::DoCallNativeSetterInfo] → [jsdbg2] Push an actual frame in ICCall_Native stubs for debug mode OSR
Push a real frame in ICCall_Native. This incidentally lets us save a little
memory by reusing a generic native wrapper.
Attachment #8413161 - Flags: review?(jdemooij)
Assignee: nobody → shu
Status: NEW → ASSIGNED
Good news is this patch doesn't really affect perf.

SunSpider, WITH patch:

============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total:                 149.0ms +/- 4.7%
--------------------------------------------

  3d:                   25.2ms +/- 1.6%
    cube:                9.2ms +/- 2.7%
    morph:               4.1ms +/- 3.9%
    raytrace:           12.0ms +/- 3.5%

  access:               14.0ms +/- 39.3%
    binary-trees:        2.0ms +/- 2.1%
    fannkuch:            7.4ms +/- 73.5%
    nbody:               2.3ms +/- 1.5%
    nsieve:              2.3ms +/- 2.2%

  bitops:                7.2ms +/- 1.0%
    3bit-bits-in-byte:   0.9ms +/- 2.2%
    bits-in-byte:        1.8ms +/- 3.7%
    bitwise-and:         1.6ms +/- 1.9%
    nsieve-bits:         2.9ms +/- 1.1%

  controlflow:           1.9ms +/- 1.7%
    recursive:           1.9ms +/- 1.7%

  crypto:               12.5ms +/- 2.8%
    aes:                 6.2ms +/- 4.7%
    md5:                 3.5ms +/- 1.8%
    sha1:                2.9ms +/- 0.9%

  date:                 18.9ms +/- 1.3%
    format-tofte:        9.7ms +/- 1.7%
    format-xparb:        9.2ms +/- 1.8%

  math:                  9.8ms +/- 1.4%
    cordic:              2.1ms +/- 1.8%
    partial-sums:        6.2ms +/- 1.5%
    spectral-norm:       1.5ms +/- 1.9%

  regexp:               10.7ms +/- 1.5%
    dna:                10.7ms +/- 1.5%

  string:               48.8ms +/- 1.7%
    base64:              4.7ms +/- 0.9%
    fasta:               5.8ms +/- 1.6%
    tagcloud:           14.4ms +/- 1.9%
    unpack-code:        16.5ms +/- 3.3%
    validate-input:      7.4ms +/- 0.9%

SunSpider, WITHOUT patch:


============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total:                 149.7ms +/- 4.1%
--------------------------------------------

  3d:                   25.6ms +/- 2.3%
    cube:                9.4ms +/- 3.4%
    morph:               4.0ms +/- 1.9%
    raytrace:           12.2ms +/- 4.3%

  access:               14.1ms +/- 36.8%
    binary-trees:        2.0ms +/- 1.7%
    fannkuch:            7.5ms +/- 69.1%
    nbody:               2.3ms +/- 1.5%
    nsieve:              2.3ms +/- 2.0%

  bitops:                7.2ms +/- 1.3%
    3bit-bits-in-byte:   0.9ms +/- 2.2%
    bits-in-byte:        1.8ms +/- 3.7%
    bitwise-and:         1.7ms +/- 2.6%
    nsieve-bits:         2.9ms +/- 1.0%

  controlflow:           1.9ms +/- 2.0%
    recursive:           1.9ms +/- 2.0%

  crypto:               12.6ms +/- 2.2%
    aes:                 6.2ms +/- 4.4%
    md5:                 3.5ms +/- 1.9%
    sha1:                2.9ms +/- 2.0%

  date:                 19.3ms +/- 1.1%
    format-tofte:        9.8ms +/- 1.5%
    format-xparb:        9.5ms +/- 1.4%

  math:                  9.7ms +/- 1.3%
    cordic:              2.1ms +/- 3.1%
    partial-sums:        6.1ms +/- 1.3%
    spectral-norm:       1.4ms +/- 2.8%

  regexp:               10.7ms +/- 1.6%
    dna:                10.7ms +/- 1.6%

  string:               48.7ms +/- 1.8%
    base64:              4.8ms +/- 1.7%
    fasta:               5.8ms +/- 1.0%
    tagcloud:           14.5ms +/- 2.7%
    unpack-code:        16.4ms +/- 3.9%
    validate-input:      7.3ms +/- 1.1%

Octane, WITH patch:

Richards: 21866
DeltaBlue: 30467
Crypto: 26340
RayTrace: 52021
EarleyBoyer: 32509
RegExp: 2855
Splay: 15687
SplayLatency: 10531
NavierStokes: 31398
PdfJS: 15762
Mandreel: 30692
MandreelLatency: 38348
Gameboy: 48635
CodeLoad: 19178
Box2D: 32532
zlib: 81501
Typescript: 21180
----
Score (version 9): 24549

Octane, WITHOUT patch:

Richards: 21887
DeltaBlue: 29575
Crypto: 26542
RayTrace: 51281
EarleyBoyer: 32580
RegExp: 2793
Splay: 16323
SplayLatency: 11431
NavierStokes: 31335
PdfJS: 15851
Mandreel: 30177
MandreelLatency: 37483
Gameboy: 45719
CodeLoad: 19183
Box2D: 32877
zlib: 81112
Typescript: 20746
----
Score (version 9): 24482
Keywords: sec-low
I'll also add assertions that, while patching return addresses for debug mode OSR, each frame's return address is different.
I don't really understand the patch. AFAICS, the problem is this:

(1) There's a Call_Native stub for the call to evaluate(..)
(2) Debug mode OSR patches the stub pointer in the stub frame to the Call_Fallback stub.
(3) The callWithABI returns to the Call_Native stub, where it crashes in the code emitted for EmitEnterTypeMonitorIC(masm), because the stub pointer is not a Call_Native stub but a Call_Fallback stub.

What happens with this patch instead of (3)? Will we return to the returnFromStubOffset_ code in Call_Fallback? The code there looks wrong for native calls.

Could the debug OSR code attach a Call_Native stub in this case, based on the old one? Then (3) wouldn't crash and it's zero overhead in the no-debug-mode case.

(In reply to Shu-yu Guo [:shu] from comment #6)
> Push a real frame in ICCall_Native. This incidentally lets us save a little
> memory by reusing a generic native wrapper.

There's only a single copy of the Call_Native stub code so this doesn't matter.
Attachment #8413161 - Flags: review?(jdemooij)
This patch reimplements how stub frames are patched. Instead of going to some
special fixer like the Ion bailout fixer, this patch clones optimized stubs on
the stack and uses the old stub's JitCode, thus keeping it alive. Fallback
stubs don't need to be cloned, since they always exist.

This should be cleaner than the old approach and easier to reason about, since
we will always return to the same stub code.
Attachment #8414085 - Flags: review?(jdemooij)
Part 1 obviates the need for a special return-from-stub fixer originally added
to patch stub frames, so I reverted the change.
Attachment #8414086 - Flags: review?(jdemooij)
Attached patch TestSplinter Review
Attachment #8414087 - Flags: review?(jdemooij)
Attachment #8413161 - Attachment is obsolete: true
Summary: [jsdbg2] Push an actual frame in ICCall_Native stubs for debug mode OSR → [jsdbg2] Clone optimized ICStubs when doing debug mode OSR
Comment on attachment 8414085 [details] [diff] [review]
Part 1: Clone on-stack optimized stubs when doing debug mode OSR

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

::: js/src/jit/BaselineJIT.cpp
@@ +638,5 @@
>  
>  void
> +BaselineScript::adoptFallbackStubsForDebugModeOSR(BaselineScript *other)
> +{
> +}

Oops, forgot to remove this.

::: js/src/jit/BaselineJIT.h
@@ +304,5 @@
>      }
>  
>      void copyICEntries(JSScript *script, const ICEntry *entries, MacroAssembler &masm);
>      void adoptFallbackStubs(FallbackICStubSpace *stubSpace);
> +    void adoptFallbackStubsForDebugModeOSR(BaselineScript *other);

And this. Ignore these please.
Comment on attachment 8414085 [details] [diff] [review]
Part 1: Clone on-stack optimized stubs when doing debug mode OSR

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

Great, r=me with nits addressed and changes to BaselineJIT.* reverted.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include <malloc.h>

Is this include necessary?

@@ +581,5 @@
>      for (size_t i = 0; i < entries.length(); i++) {
>          JSScript *script = entries[i].script;
> +
> +        if (!RecompileBaselineScriptForDebugMode(cx, script) ||
> +            !CloneOldBaselineStub(cx, entries[i]))

What happens if a single stub is on the stack multiple times, will we clone it multiple times? It's harmless but would be better to avoid it.

::: js/src/jit/BaselineIC.h
@@ +3166,5 @@
>      }
> +
> +    static ICGetElem_NativePrototypeCallNative *Clone(
> +             JSContext *cx, ICStubSpace *space, ICStub *firstMonitorStub,
> +             ICGetElem_NativePrototypeCallNative &other)

The indentation here looks unusual. Could you also move the definition to the cpp file? There's too much code in the header file (same for MIR.h), I've been trying to add more to the cpp file. This also doesn't need inlining etc.

Same for the methods below.
Attachment #8414085 - Flags: review?(jdemooij) → review+
Comment on attachment 8414086 [details] [diff] [review]
Part 2: Revert bailout return offset changes in Baseline can-call fallbacks

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

\o/
Attachment #8414086 - Flags: review?(jdemooij) → review+
Attachment #8414087 - Flags: review?(jdemooij) → review+
landed on m-c 
https://hg.mozilla.org/mozilla-central/rev/e3480e87428d
https://hg.mozilla.org/mozilla-central/rev/caad9ce31941
https://hg.mozilla.org/mozilla-central/rev/c3c7a4b24fbe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.