Closed Bug 1006899 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
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


(Reporter: decoder, Assigned: nbp)



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

Crash Data


(2 files, 2 obsolete files)

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

  function() {
    return this;
function callback(obj) {}
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:
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
Attachment #8433377 - 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
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, &;
>      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]:
(running on inbound atm)

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

[String/UUID change made/needed]:
Attachment #8453150 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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.

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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.