688373 - Write regression test for: Assertion failure: unsigned(sp - entries) < feLimit(), at methodjit/FrameState-inl.h:196 or Crash [@ js::mjit::FrameState::discardFrame]

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test receives a glibc abort on Linux (due to memory corruption) with mozilla-aurora revision 009c64b64cf3 (options -j -m -a):


var f = function() { 
        L: if (0) for (var f in printStatus)
        L[summary++] = f; 
}
new f;


The test does not reproduce on mozilla-central (maybe because TI has changed the way how mozilla-central works?). The abort looks like this:

$ $JS $JSOPTS min.js
*** glibc detected *** /srv/repos/mozilla-aurora/js/src/opt64/js: malloc(): memory corruption: 0x0000000001fa6830 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x79d7a)[0x7f61e04a8d7a]
/lib/x86_64-linux-gnu/libc.so.6(__libc_malloc+0x6e)[0x7f61e04ab31e]
[...]

When run in valgrind, I get a crash with a very long list of errors indicating the memory corruption, including use after free, ending in general protection fault.

Comment 1

[Daniel Veditz [:dveditz]]

Was the bug actually fixed in TI, or simply masked such that a variant can disable whatever TI does and get something closer to JM behavior?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

This assert stopped appearing with e5d548c51427, which rejiggered a lot of stuff in the method JIT. The bytecode to this function is just:
   nop
   nop
   stop

nfixed == 1, nslots == 1

But it's called as a constructor so nslots needs to be 2. A quick fix: Always add 1 to script->nslots (not sure how JIT'ing ctors works without that, really). If that's not palatable, we could over-fill the FrameEntry's slots requirement, pretending there's an additional stack slot - and if we ever actually go over the limit, abort compilation at the end.

Comment 3

[David Mandelin [:dmandelin]]

(In reply to David Anderson [:dvander] from comment #2)
> This assert stopped appearing with e5d548c51427, which rejiggered a lot of
> stuff in the method JIT. 

Does that mean the bug went away, or just an assert?

> The bytecode to this function is just:
>    nop
>    nop
>    stop
> 
> nfixed == 1, nslots == 1
> 
> But it's called as a constructor so nslots needs to be 2. A quick fix:
> Always add 1 to script->nslots (not sure how JIT'ing ctors works without
> that, really). If that's not palatable, we could over-fill the FrameEntry's
> slots requirement, pretending there's an additional stack slot - and if we
> ever actually go over the limit, abort compilation at the end.

Comment 4

[Christian Holler (:decoder)]

This does not only affect beta, but also stable. I tried with JS beta/release shells:

Both debug shells give me this assertion:

Assertion failure: unsigned(sp - entries) < feLimit(), at /srv/repos/mozilla-release/js/src/methodjit/FrameState-inl.h:196

The beta optimized build crashes as in my first comment, while the release shell crashes and valgrind gives a lot of invalid reads first:

==52288== Invalid read of size 4
==52288==    at 0x5D37E9: js::mjit::FrameState::pop() (FastOps.cpp:1849)
==52288==    by 0x5C750E: js::mjit::Compiler::constructThis() (Compiler.cpp:4913)
[...]
==52288==  Address 0x5e09004 is 3 bytes after a block of size 241 alloc'd
==52288==    at 0x4C279FC: calloc (vg_replace_malloc.c:467)
==52288==    by 0x5D5D3A: js::mjit::FrameState::init() (jsutil.h:235)
[...]

then crashes with invalid write:

==52288== Invalid write of size 1
==52288==    at 0x5D5F3A: js::mjit::FrameState::discardFrame() (FrameEntry.h:238)
==52288==    by 0x5C36F7: js::mjit::Compiler::emitReturn(js::mjit::FrameEntry*) (Compiler.cpp:2300)
[...]
==52288==  Address 0xfffb800005e09022 is not stack'd, malloc'd or (recently) free'd

Comment 5

[Daniel Veditz [:dveditz]]

Decoder: Please test mozilla-central again to make sure this is still masked/fixed.

If it is then we should morph this bug to "check this in as a regression test" to make sure the problem doesn't come back. Since it's gone away in Fx9 as long as it stays away it's probably not high priority to track this down and fix in Fx8. If there's still a hidden masked problem hopefully the fuzzers will expose it in the future.

Comment 6

[Christian Holler (:decoder)]

(In reply to Daniel Veditz from comment #5)
> If it is then we should morph this bug to "check this in as a regression
> test" to make sure the problem doesn't come back. Since it's gone away in
> Fx9 as long as it stays away it's probably not high priority to track this
> down and fix in Fx8. If there's still a hidden masked problem hopefully the
> fuzzers will expose it in the future.

I disagree here. Of course we should transform this into a regression test, but we should also ensure that the underlying bug is not masked but really fixed here. Such bugs can be hard to detect and having a developer look at this should be much cheaper than tracking down a possible regression this might cause in the future. I will ask Brian Hackett if this issue is "fixed" by TI or just masked as he should be able to tell best I guess.

Comment 7

[Christian Holler (:decoder)]

I talked to jandem and bhackett, and according to them, JM+TI always gets extra space for loop invariant entries which means this bug does really not affect JM+TI branches.

Comment 8

[Johnny Stenback (:jst)]

I don't think it's worth holding 8 for this, so marking wontfix there. Given that trunk and 9 are unaffected here, is there any reason to keep this bug open any more?

Gary says we should have a regression test for this, I guess we can track that in this bug.

Comment 9

[Christian Holler (:decoder)]

Marking as sg:critical per comment 1.

Comment 10

[Johnny Stenback (:jst)]

I think all that's needed here is to add a regression test.

Comment 11

[Jan de Mooij [:jandem]]

JM was removed, see also comment 7 (should not affect other branches).