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

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

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 });

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

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

Comment 3

[Christian Holler (:decoder)]

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

Comment 4

[Christian Holler (:decoder)]

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.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error (try manually).

Comment 6

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 7

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 8

[Christian Holler (:decoder)]

Needinfo on Brian and Jandem to figure out who might be able to fix this. It still triggers all the time.

Comment 9

[Jan de Mooij [:jandem]]

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.

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

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.

Comment 11

[Nicolas B. Pierron [:nbp] {away}]

Attachment: Prevent stack iterations while recovering allocations.

Comment 12

[Brian Hackett [Laid off!]]

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.

Comment 13

[Christian Holler (:decoder)]

Marked s-s per nbp's comment on IRC.

Comment 14

[Nicolas B. Pierron [:nbp] {away}]

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?

Comment 15

[Brian Hackett [Laid off!]]

The object metadata callback is only used for devtools stuff, and not during normal browsing.

Comment 16

[Nicolas B. Pierron [:nbp] {away}]

(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.

Comment 17

[Nicolas B. Pierron [:nbp] {away}]

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?

Comment 18

[Al Billings [:abillings - ex-MoCo]]

I'm not sure that we need to keep this hidden.

What is the worst case scenario here for impact?

Comment 19

[Nicolas B. Pierron [:nbp] {away}]

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.

Comment 20

[Daniel Veditz [:dveditz]]

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.

Comment 21

[Nicolas B. Pierron [:nbp] {away}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe1e613cf21

Comment 22

[Nicolas B. Pierron [:nbp] {away}]

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

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fe1e613cf21

Comment 24

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

Checked on IRC with Nicolas. Also accept for beta.

Comment 26

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/802588f871fc
https://hg.mozilla.org/releases/mozilla-beta/rev/b801d912ea0e

Comment 27

[Ryan VanderMeulen (PTO, back July 27)]

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.

Comment 28

[Ryan VanderMeulen (PTO, back July 27)]

Pushed to m-c as a ride-along with the most recent set of merges.
https://hg.mozilla.org/mozilla-central/rev/e95658da5a3d

Comment 29

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b801d912ea0e
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/794d229b5125