Crash [@ js::jit::JitFrameIterator::operator++] with setObjectMetadataCallback

VERIFIED FIXED in Firefox 32

Status

()

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

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks 1 bug, {crash, sec-moderate, testcase})

Trunk
mozilla34
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30 unaffected, firefox31 unaffected, firefox32 fixed, firefox33 fixed, firefox34 verified, firefox-esr24 unaffected, firefox-esr31 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [jsbugmon:update] sec-high if not restricted to devtools, crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

The following testcase crashes on mozilla-central revision 87c8f870e2b9 (run with --fuzzing-safe --ion-compile-try-catch --ion-eager):


this.__defineGetter__("x",
  function() {
    return this;
  }
);
function callback(obj) {}
setObjectMetadataCallback(callback);
evaluate("\
var { ArrayType, StructType, uint32 } = TypedObject;\
  var L = 1024;\
  var Matrix = uint32.array(L, 2);\
  var matrix = new Matrix();\
  for (var i = 0; i < L; i++)\
    matrix[i][0] = x;\
", { compileAndGo : true });
Crash trace:


Program received signal SIGSEGV, Segmentation fault.
js::jit::JitFrameIterator::operator++ (this=0x7fffffffa708) at jit/IonFrames.cpp:316
316         frameSize_ = prevFrameLocalSize();
#0  js::jit::JitFrameIterator::operator++ (this=0x7fffffffa708) at jit/IonFrames.cpp:316
#1  0x0000000000999add in js::FrameIter::settleOnActivation (this=0x7fffffffa6b0) at vm/Stack.cpp:576
#2  0x000000000048eeda in ScriptFrameIter (this=0x7fffffffa6b0, savedOption=js::FrameIter::STOP_AT_SAVED, cx=0x1840c60) at vm/Stack.h:1734
#3  NonBuiltinScriptFrameIter (opt=js::FrameIter::STOP_AT_SAVED, cx=0x1840c60, this=0x7fffffffa6b0) at vm/Stack.h:1826
#4  ShellObjectMetadataCallback (cx=0x1840c60, pmetadata=0x7fffffffabd0) at builtin/TestingFunctions.cpp:1146
#5  0x000000000087e8d0 in callObjectMetadataCallback (obj=0x7fffffffabd0, cx=0x1840c60, this=<optimized out>) at jscompartment.h:346
#6  NewObjectMetadata (pmetadata=0x7fffffffabd0, cxArg=0x1840c60) at jsobjinlines.h:1066
#7  NewObject (cx=0x1840c60, type_=<optimized out>, parent=0x7ffff5642060, kind=js::gc::FINALIZE_OBJECT8_BACKGROUND, newKind=js::GenericObject) at jsobj.cpp:1253
rax     0x8     8
rbx     0xffffa708      140737488332552
rcx     0x0     0
rdx     0x0     0
rsi     0xffffa6f8      140737488332536
rdi     0xffffa708      140737488332552
rbp     0xffffa4e0      140737488332000
rsp     0xffffa4c0      140737488331968
r8      0x0     0
r9      0x1     1
r10     0x0     -1970324836974592
r11     0x4000000       67108864
r12     0xffffa708      140737488332552
r13     0x0     0
r14     0x1840c60       25431136
r15     0xffffa6d8      140737488332504
rip     0x652704 <js::jit::JitFrameIterator::operator++()+36>
=> 0x652704 <js::jit::JitFrameIterator::operator++()+36>:       mov    0x8(%r13),%r12
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140424020245" and the hash "079f4f0ed6a6".
The "bad" changeset has the timestamp "20140424022242" and the hash "462059b115ec".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=079f4f0ed6a6&tochange=462059b115ec
Still hitting this all the time, needinfo on Brian now since it involves setObjectMetadataCallback.

Brian, any idea who could look at this?

The bisect looks bogus, I'm going to try a compiled bisect.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,bisect,bisect-force-compile]
Whiteboard: [jsbugmon:update,bisect,bisect-force-compile] → [jsbugmon:update]
JSBugMon: Bisection requested, failed due to error (try manually).
Attachment #8418429 - Attachment is obsolete: true
Needinfo on Brian and Jandem to figure out who might be able to fix this. It still triggers all the time.
Flags: needinfo?(jdemooij)
Flags: needinfo?(bhackett1024)
The second stack trace (see attachment) points to the RNewObject recover instruction. The shell's object metadata callback then tries to get a stack trace but this fails because we're in a bailout.
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
I was able to reproduce this issue with both optimized x86 and optimized x64 builds on Linux.  No assertion / segv on debug builds.

Even if this might hurt correctness in a few cases (when Ion guesses are incorrect), I think preventing such callback to be executed on RNewObject* is likely to be a good way to go, as RNewObject* are generated when Ion notices that the object is unused / not-escaped.

The other options are to enable stack iterations on a bailing stack frames, or to prevent any form of stack iterations while we are in a bailout, in which case the correctness issue would be restricted to a missing stack trace instead of a missing callback call.
Blocks: 1005532, 1004527
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(bhackett1024)
Hardware: x86 → All
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

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

::: js/src/jit/Recover.cpp
@@ +850,5 @@
>      RootedObject templateObject(cx, &iter.read().toObject());
>      RootedValue result(cx);
>      JSObject *resultObject = nullptr;
>  
> +    // Use AutoEnterAnalysis to prohibit any stack iteration during a bailout.

AutoEnterAnalysis doesn't have any direct effect on stack iteration, so how about:

// Use AutoEnterAnalysis to avoid invoking the object metadata callback while bailing out, which could try to walk the stack.
Attachment #8453150 - Flags: review?(bhackett1024) → review+
Marked s-s per nbp's comment on IRC.
Group: core-security
The setObjectMetadataCallback is only used in Testing functions?  What is this function supposed to mirror?  Dev-tools introspection? GC?

Do we have anything similar which can be impacted if we do not backport this change?
Flags: needinfo?(bhackett1024)
The object metadata callback is only used for devtools stuff, and not during normal browsing.
Flags: needinfo?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #15)
> The object metadata callback is only used for devtools stuff, and not during
> normal browsing.

Thanks, this means that we should backport this patch to 32.
If This is only used by dev-tools and only exposed to privileged JS, should we keep the s-s flag?  Should I asked for sec-approval and strip the comments out of the patch?
Flags: needinfo?(abillings)
I'm not sure that we need to keep this hidden.

What is the worst case scenario here for impact?
Flags: needinfo?(abillings)
I guess the worse is that we might crash when a developer requests to find the stack traces of all objects allocations, and during a bailout we need to reconstruct an empty object or a typed object.
This is sec-high or -critical lowered to sec-moderate because it's not accessible to the web. The comments in the patch are fine.
Keywords: sec-moderate
Whiteboard: [jsbugmon:update] → [jsbugmon:update] sec-high if not restricted to devtools
Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1004527, Bug 1005532 

[User impact if declined]:
Crash while inspecting allocations during dev-tools.

[Describe test coverage new/current, TBPL]:
https://treeherder.allizom.org/ui/#/jobs?repo=try&revision=d5fbd9f5a8d8
(running on inbound atm)

[Risks and why]:
This patch prevent calling the function which might crash. Low risk.

[String/UUID change made/needed]:
None
Attachment #8453150 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9fe1e613cf21
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Attachment #8453150 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

Checked on IRC with Nicolas. Also accept for beta.
Attachment #8453150 - Flags: approval-mozilla-beta+
Follow-up fix for the test to early-quit if TypedObject isn't enabled.

https://hg.mozilla.org/releases/mozilla-aurora/rev/40218ed779cb
https://hg.mozilla.org/releases/mozilla-beta/rev/794d229b5125

I'll push it to inbound as a ride-along next time I'm pushing there.
Pushed to m-c as a ride-along with the most recent set of merges.
https://hg.mozilla.org/mozilla-central/rev/e95658da5a3d
Depends on: 1050285
Group: core-security
You need to log in before you can comment on or make changes to this bug.