867756 - Intl: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:398 or Crash [@ js::ion::CodeGenerator::link]

Status: RESOLVED DUPLICATE of bug 877287

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 02aa81c59df6 (run with --ion-eager):


for (var n = 0; n < 100; n++) { Intl++; }

Comment 1

[Christian Holler (:decoder)]

Crash trace:

Program received signal SIGSEGV, Segmentation fault.
js::ion::CodeGenerator::link (this=0x18d96d0) at js/src/ion/CodeGenerator.cpp:4942
4942        if (cx->compartment->types.compiledInfo.compilerOutput(cx)->isInvalidated())
(gdb) bt
#0  js::ion::CodeGenerator::link (this=0x18d96d0) at js/src/ion/CodeGenerator.cpp:4942
#1  0x00000000006efa16 in compile (autoDelete=<synthetic pointer>, builder=0x16eb770, this=<optimized out>, graph=<optimized out>) at js/src/ion/Ion.cpp:1433
#2  js::ion::IonCompile<js::ion::SequentialCompileContext> (cx=<optimized out>, script=<optimized out>, fp=..., osrPc=0x16c9d8b  <incomplete sequence \343\232>, constructing=<optimized out>, compileContext=...)
    at js/src/ion/Ion.cpp:1367
#3  0x00000000006efb7d in js::ion::Compile<js::ion::SequentialCompileContext> (cx=<optimized out>, script=0x7ffff654a128, fp=..., osrPc=<optimized out>, constructing=<optimized out>, compileContext=...)
    at js/src/ion/Ion.cpp:1598
#4  0x00000000006f1e64 in js::ion::CanEnterAtBranch (cx=0x16cf2e0, script=0x7ffff654a128, fp=..., pc=0x16c9d8b  <incomplete sequence \343\232>, isConstructing=<optimized out>)
    at js/src/ion/Ion.cpp:1643
#5  0x00000000008d9d97 in EnsureCanEnterIon (jitcodePtr=<synthetic pointer>, pc=0x16c9d8b  <incomplete sequence \343\232>, script=0x7ffff654a128, frame=0x7fffffffd388, cx=0x16cf2e0, stub=<optimized out>)
    at js/src/ion/BaselineIC.cpp:658
#6  DoUseCountFallback (infoPtr=0x7fffffffd360, frame=0x7fffffffd388, stub=<optimized out>, cx=0x16cf2e0) at js/src/ion/BaselineIC.cpp:844
#7  js::ion::DoUseCountFallback (cx=0x16cf2e0, stub=<optimized out>, frame=0x7fffffffd388, infoPtr=0x7fffffffd360) at js/src/ion/BaselineIC.cpp:803
#8  0x00007ffff67c81a8 in ?? ()
[...]
(gdb) x /i $pc
=> 0x940679 <js::ion::CodeGenerator::link()+217>:       cmpq   $0x0,(%rcx)
(gdb) info reg rcx
rcx            0x0      0


Crashes at NULL, but I know that the assertion can indicate security problems, so filing s-s until triaged by a developer. Ccing Waldo because Intl is involved.

Comment 2

[Robert Kaiser]

What versions are affected by this?

Comment 3

[Christian Holler (:decoder)]

Confirmed that this is probably sec-critical, ASan showed this for a test yielding the same assertion/crash on a normal build:

==22848== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60720006c070 at pc 0x114fbfc bp 0x7fff1b332310 sp 0x7fff1b332308
READ of size 8 at 0x60720006c070 thread T0
    #0 0x114fbfb in js::types::RecompileInfo::compilerOutput(JSContext*) const js/src/opt64asan/../jsinfer.h:1307
    #1 0xc3ac44 in js::ion::SequentialCompileContext::compile(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ScopedJSDeletePtr<js::LifoAlloc>&) js/src/ion/Ion.cpp:1433
    #2 0xc3d0ba in js::ion::AbortReason js::ion::IonCompile<js::ion::SequentialCompileContext>(JSContext*, JSScript*, js::AbstractFramePtr, unsigned char*, bool, js::ion::SequentialCompileContext&) js/src/ion/Ion.cpp:1367
    #3 0xc3e37f in js::ion::CompileFunctionForBaseline(JSContext*, JS::Handle<JSScript*>, js::AbstractFramePtr, bool) js/src/ion/Ion.cpp:1734
    #4 0x10605d0 in js::ion::EnsureCanEnterIon(JSContext*, js::ion::ICUseCount_Fallback*, js::ion::BaselineFrame*, JS::Handle<JSScript*>, unsigned char*, void**) js/src/ion/BaselineIC.cpp:667
    #5 0x7faa63ba0598 in
0x60720006c070 is located 144 bytes to the left of 4096-byte region [0x60720006c100,0x60720006d100)
allocated by thread T0 here:
    #0 0x4a8063 in calloc ??:0
    #1 0x5626df in js_calloc(unsigned long) js/src/opt64asan/./dist/include/js/Utility.h:157
Shadow bytes around the buggy address:
  0x1c0e4000d7b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c0e4000d7c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c0e4000d7d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c0e4000d7e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0e4000d7f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x1c0e4000d800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa
  0x1c0e4000d810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0e4000d820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c0e4000d830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c0e4000d840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c0e4000d850: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   130388:533d3fb8a7e9
user:        Norbert Lindenberg
date:        Tue Apr 30 16:28:58 2013 -0400
summary:     Bug 866305 - Enable ECMAScript Internationalization API for JavaScript standalone build. r=Waldo, r=glandium

This iteration took 336.867 seconds to run.

Comment 5

[Jeff Walden [:Waldo]]

This doesn't look like it should be Intl-specific to me.  Could be self-hosting-specific, somehow (but Intl itself is an object created in C++, so I dunno).

Comment 6

[Andrew McCreight [:mccr8]]

till: waldo thinks this may be self-hosting related, which is why I CC'ed you.

Comment 7

[Christian Holler (:decoder)]

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

Comment 8

[Christian Holler (:decoder)]

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

Comment 9

[Christian Holler (:decoder)]

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

Comment 10

[Till Schneidereit [:till]]

So this is pretty weird. I don't think it's related to self-hosting per-se, but something weird is going on with the Intl object's initialization.

As the standard classes get resolved lazily, Intl is initialized upon first access. In this case, this happens to be caused from within an ion compilation. Specifically, IonBuilder traverses the currently compiled script's opcodes, and this triggers the standard class to get resolved.

I guess this scenario hasn't ever occurred before because, AFAICT, resolve hooks are the only way to make it happen. At least in this precise manner.

@bhackett, do you know why we assert that no analysis is happening in js::Invoke, and whether we need to?

Comment 11

[Christian Holler (:decoder)]

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

Comment 12

[Christian Holler (:decoder)]

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

Comment 13

[Christian Holler (:decoder)]

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

Comment 14

[Brian Hackett [Laid off!]]

JS can't be reentered in the middle of analysis, as it could cause horrible things like reentrant compilation and types to be in an inconsistent state.  It looks like Intl is the first standard class that enters JS during initialization.

That behavior should be fine, we just need to make sure that Ion doesn't actually trigger global class initialization.  The code that is supposed to guard this is CanEffectlesslyCallLookupGenericOnObject in IonBuilder.cpp, but that doesn't look at resolve hooks on the clasp.  This could be fixed I think either by adding a test for resolve hooks here, or for global objects.  The latter would be better but might affect Dromaeo.

Comment 15

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #14)
> This could be fixed I
> think either by adding a test for resolve hooks here, or for global objects.
> The latter would be better but might affect Dromaeo.

Er, this should be 'the *former* would be better'

Comment 16

[Christian Holler (:decoder)]

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

Comment 17

[Till Schneidereit [:till]]

(In reply to Brian Hackett (:bhackett) from comment #14)
> That behavior should be fine, we just need to make sure that Ion doesn't
> actually trigger global class initialization.  The code that is supposed to
> guard this is CanEffectlesslyCallLookupGenericOnObject in IonBuilder.cpp,
> but that doesn't look at resolve hooks on the clasp.  This could be fixed I
> think either by adding a test for resolve hooks here, or for global objects.
> The latter would be better but might affect Dromaeo.

What about just disabling lazy class resolution when --ion-eager or --baseline-eager is activated? Under these conditions the error shouldn't show up at all, right?

I guess having a somewhat different configuration of the rest of the VM depending on when the JITs get activated is kinda bad, though ...

Comment 18

[Christian Holler (:decoder)]

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

Comment 19

[Christian Holler (:decoder)]

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

Comment 20

[Christian Holler (:decoder)]

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

Comment 21

[Christian Holler (:decoder)]

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

Comment 22

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9ca690835a5e).

Comment 23

[Christian Holler (:decoder)]

I'm pretty sure this isn't gone, but let's see what bisection says.

Comment 24

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/7df36088f645
user:        Kannan Vijayan
date:        Wed Jun 05 16:42:23 2013 -0400
summary:     Bug 877287. r=h4writer

This iteration took 343.819 seconds to run.

Comment 25

[Christian Holler (:decoder)]

Interesting, djvj, can you check if your fix did indeed fix this problem too? I've only seen this particular problem occurring with Intl.

Comment 26

[Kannan Vijayan [:djvj]]

(In reply to Christian Holler (:decoder) from comment #25)
> Interesting, djvj, can you check if your fix did indeed fix this problem
> too? I've only seen this particular problem occurring with Intl.

Since Brian already did the work to identify the issue here, this question is pretty easy to answer.  What bhackett described was what I fixed with that patch :)

Namely:
(In reply to Brian Hackett (:bhackett) from comment #14)
> That behavior should be fine, we just need to make sure that Ion doesn't
> actually trigger global class initialization.  The code that is supposed to
> guard this is CanEffectlesslyCallLookupGenericOnObject in IonBuilder.cpp,
> but that doesn't look at resolve hooks on the clasp.  This could be fixed I
> think either by adding a test for resolve hooks here, or for global objects.
> The latter would be better but might affect Dromaeo.

Comment 27

[Christian Holler (:decoder)]

Thanks djvj!