Closed Bug 610498 Opened 9 years ago Closed 9 years ago

JM crash in JSString::length

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jandem, Assigned: dvander)

References

Details

(Whiteboard: [sg:critical][fixed-in-tracemonkey])

Attachments

(2 files)

Attached file Test case
The attached shell test case crashes JM debug and release shells. It's also reproducible in a tinderbox browser build.

Stack looks like this:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xb0e23d03
0x00017e59 in JSString::length (this=0xb0e23d03) at jsstr.h:259
259	        return mLengthAndFlags >> FLAGS_LENGTH_SHIFT;
(gdb) bt
#0  0x00017e59 in JSString::length (this=0xb0e23d03) at jsstr.h:259
#1  0x00182aa0 in js_ConcatStrings (cx=0x60b2b0, left=0x1504be0, right=0xb0e23d03) at ../jsstr.cpp:284
#2  0x0024e5c9 in js::mjit::stubs::Add (f=@0xbffff4e0) at ../methodjit/StubCalls.cpp:1173
#3  0x005ec063 in ?? ()
#4  0x002439f8 in js::mjit::EnterMethodJIT (cx=0x60b2b0, fp=0x1000030, code=0x5eb044, stackLimit=0x108ca60) at ../methodjit/MethodJIT.cpp:745
#5  0x00243b0f in CheckStackAndEnterMethodJIT (cx=0x60b2b0, fp=0x1000030, code=0x5eb044) at ../methodjit/MethodJIT.cpp:770
#6  0x00243c31 in js::mjit::JaegerShot (cx=0x60b2b0) at ../methodjit/MethodJIT.cpp:787
#7  0x000e2cb1 in js::RunScript (cx=0x60b2b0, script=0x613040, fp=0x1000030) at jsinterp.cpp:662
#8  0x000e3282 in js::Execute (cx=0x60b2b0, chain=0x1502028, script=0x613040, prev=0x0, flags=0, result=0x0) at jsinterp.cpp:1028
#9  0x000222fc in JS_ExecuteScript (cx=0x60b2b0, obj=0x1502028, script=0x613040, rval=0x0) at ../jsapi.cpp:4825
Summary: JM crashes on the attached test case → JM crash in JSString::length
Sounds either like something is mistagged or the syncing process is going bad. We should see if bug 609970 (636b1e5a994f) regressed this.
blocking2.0: --- → beta8+
(In reply to comment #1)
> Sounds either like something is mistagged or the syncing process is going bad.
> We should see if bug 609970 (636b1e5a994f) regressed this.

It already crashes on the version just before that.
Assignee: general → dvander
FWIW, here is another one:
----
function a1() {
    var a2 = "123";
    var a3 = a2 ? 1 : 0;
    ((a2 !== false) >= a2);
}
a1();
----
Stack is a bit different:
----
#0  0x00018a59 in JSString::length (this=0x8) at jsstr.h:253
#1  0x000f329d in js::StringToNumberType<double> (cx=0x60b2a0, str=0x8) at jsnum.h:652
#2  0x000f350c in js::ValueToNumberSlow (cx=0x60b2a0, v={data = {asBits = 18446462620207677448, s = {payload = {i32 = 8, u32 = 8, boo = 8, str = 0x8, obj = 0x8, ptr = 0x8, why = JS_SERIALIZE_NO_NODE, word = 8}, tag = JSVAL_TAG_STRING}, asDouble = -nan(0xf000500000008), asPtr = 0x8}}, out=0xbffff4c8) at ../jsnum.cpp:1248
----
worst case may be sg:critical if we do something else with this invalid string? Is the worst case reading private data out of memory? If so maybe sg:high or sg:moderate?
Whiteboard: [sg:critical?]
Attached patch fixSplinter Review
Great catch, Jan. This is a bug in some very old code, which allocates a register inside a local branch. If the branch isn't taken, the register is garbage.

This is sg:critical, since it could be used to inject an arbitrary payload as an object pointer.
Attachment #489568 - Flags: review?(dmandelin)
Duplicate of this bug: 610428
Comment on attachment 489568 [details] [diff] [review]
fix

Looks good to me. But let me test my understanding:

- The problem was that 'data' was allocated, so below the tracker thought it had a certain value, but this was only true if the local branch where data was set/allocated was taken.

- In the solution, you now allocate 'data' before the branch, using pinning so that it is guaranteed to be a register after that block. (Small question: why is that better than simply allocating 'data' after 'result'?)

- Nothing that allocates regs occurs between that point and the one where the value is set, so you can simply store from that register, without the old logic.

Is that correct?
Attachment #489568 - Flags: review?(dmandelin) → review+
Yeah, re: the above. The pinning and re-ordering avoids the possibility of the initial allocation evicting data, and then data having to be loaded again.

http://hg.mozilla.org/tracemonkey/rev/c938c2dc5f37
Status: NEW → ASSIGNED
Whiteboard: [sg:critical?] → [sg:critical][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/c938c2dc5f37
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 613764
(In reply to comment #6)
> This is a bug in some very old code [...]

Jaegermonkey old, or affects-1.9-branches old?
status1.9.1: --- → ?
status1.9.2: --- → ?
No longer depends on: 613764
Depends on: 613764
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.