627783 - JM: Crash at a weird memory address or "Assertion failure: (inst & mask) == expected," on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: not fully reduced testcase that crashes opt shell occasionally

(function () {
    function a() {
        *::*
    }
    for each (let w in [
        String(), String(), String(), String(), String(), String()
    ]) {
        try {
            a()
        } catch (e) {}
    }
}())


asserts js debug shell on TM changeset 284811f39ca6 with -m at Assertion failure: (inst & mask) == expected. The attached file is a testcase that occasionally spouts a crash at a weird memory address on a js opt shell, and asserts identically.

Locking s-s because I don't know if this can be manipulated even though it's on ARM, just to be safe.

First jsfunfuzz bug found on a Tegra 250 Ubuntu Linux 9.04!

Does not seem to affect Mac js shells on TM changeset e63f5adc4280.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm not sure if I should nominate blocking2.0? for an ARM bug but probably should because the final release of Firefox for Mobile is coming out around the same time..

Comment 3

[Jacob Bramley [:jbramley]]

This looks like a PIC repatching bug. I'll look into it.

Comment 4

[Jacob Bramley [:jbramley]]

Confirmed on tip (a7d178f043db). It looks like the call stub compiler isn't using RESERVE_IC_SPACE to prevent a constant pool being dumped. In this case, a constant pool is being dumped in the middle of the call sequence.

Comment 5

[Jacob Bramley [:jbramley]]

Attachment: Add additional RESERVE_IC_SPACE

This fixes the given failure case. However, I get thrown out of jit-tests (with -m) with a Python exception:

----

python jit_test.py /home/jacbra01/moz/tm.d/js --jitflags=m
[  83|   0|  83]   8% ==>                                             |   39.0sTraceback (most recent call last):
  File "jit_test.py", line 457, in <module>
    main(sys.argv[1:])
  File "jit_test.py", line 446, in main
    ok = run_tests(job_list, test_dir, lib_dir)
  File "jit_test.py", line 227, in run_tests
    ok, out, err, code = run_test(test, lib_dir)
  File "jit_test.py", line 182, in run_test
    out, err, code = run_cmd(cmd, env)
  File "jit_test.py", line 134, in run_cmd
    return out.decode(), err.decode(), p.returncode
UnicodeDecodeError: 'ascii' codec can't decode byte 0xf0 in position 331500: ordinal not in range(128)

----

I should also double-check the other ICs to ensure that we haven't missed any other RESERVE_{IC,OOL}_SPACE calls.

Comment 6

[Jacob Bramley [:jbramley]]

Attachment: v2: Increases the reserved IC space.

(In reply to comment #5)
> UnicodeDecodeError: 'ascii' codec can't decode byte 0xf0 in position 331500:
> ordinal not in range(128)

Ah, I'm just being stupid. I'd left JMFLAGS=insns turned on (to check
the maximum IC space usage) and there's presumably some non-ASCII
character in its output.

The original patch failed one jit-test in debug mode because the
reserved IC space was not sufficient. The attached version allows space
as required, and adds a bit of safety space.

It should be pointed out that temporarily making this size much larger
than required for testing — 512 bytes, to be precise — appeared to have
a noticeable effect on the performance of the test suite. I only tried
it in debug mode, and my observation was not scientific. However, it's
feasible that frequent constant pool generation could have a significant
effect on performance, and we should consider ways to minimize the size
of both the protected regions and the constant pools. In any case,
that's a subject for another bug, and can probably wait until after the
release.

----

Incidentally, this is a security bug (on ARM only) as an attacker with
enough patience could use it to execute native code by putting valid
instructions in the constant pool.

I have to admit that I feel uncomfortable relying on the test suite to
stretch the IC space as far as possible when the consequence of getting
it wrong is remote code execution. Still, I haven't come up with a
better solution other than simply using the Thumb-2 back end. (The
Thumb-2 back-end doesn't use literal pools.)

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 506411 [details] [diff] [review]
v2: Increases the reserved IC space.

Looks good, and could help us fix some random crashers!

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #7)
> Comment on attachment 506411 [details] [diff] [review]
> v2: Increases the reserved IC space.
> 
> Looks good, and could help us fix some random crashers!

Can this please be checked in to TM?

Comment 9

[Jacob Bramley [:jbramley]]

(In reply to comment #8)
> Can this please be checked in to TM?

Sorry, I thought I'd done it!

Here it is: http://hg.mozilla.org/tracemonkey/rev/8dce5143d664

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/8dce5143d664