Assertion failure: JSID_BITS(id) == JSID_TYPE_VOID

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: jandem, Assigned: billm)

Tracking

unspecified
x86
All
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(blocking2.0 beta9+)

Details

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

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
Attached test case triggers this with -j in debug mode:
Assertion failure: JSID_BITS(id) == JSID_TYPE_VOID, at ../jsapi.h:452

In release mode this triggers some scary errors before it crashes:

js(86322) malloc: *** error for object 0x50a090: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
js(86322) malloc: *** error for object 0x50a0d8: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
js(86322) malloc: *** error for object 0x50a120: pointer being freed was not allocated
...repeated many times...


The weird part is that this seems to depend on the full path to the script. If I store the file as /tmp/test.js, I get the crash in release mode but not the messages and I don't get the assert in a debug build. If I store the file as /tmp/test1234567891234567890.js (length 31) I get the assert and error messages. I hope somebody will be able to reproduce.
(Reporter)

Comment 1

8 years ago
Created attachment 496850 [details]
Test case
(Reporter)

Comment 2

8 years ago
Created attachment 496854 [details]
Stack trace

Attached is the stack trace for the assert.
Stack trace in release build:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xb7e70010
0x001289f9 in js_script_filename_sweeper ()
(gdb) bt
#0  0x001289f9 in js_script_filename_sweeper ()
#1  0x00080ec2 in JS_HashTableEnumerateEntries ()

This might explain why it depends on the file name.
(Reporter)

Updated

8 years ago
Attachment #496854 - Attachment mime type: application/octet-stream → text/plain
Do you suppose you could run this under valgrind (--tool=memcheck) and hopefully catch some backtraces earlier than the crash, when the initial memory corruption occurs?
(Reporter)

Comment 4

8 years ago
I was able to reduce this to this using Valgrind (great idea!):
---
function f(p) {
  "use strict";
  return arguments;
}
for (var i = 0; i < 10; i++) {
    f({});
}
---
This triggers 2 messages like this:
==8341== Invalid write of size 4
==8341==    at 0x685CFB8: ???
==8341==    by 0x1CB1D4: js::ExecuteTrace(JSContext*, nanojit::Fragment*, js::TracerState&) (in ./js)
==8341==    by 0x1D8FA9: js::ExecuteTree(JSContext*, js::TreeFragment*, unsigned int&, js::VMSideExit**, js::VMSideExit**) (in ./js)
==8341==    by 0x1FFD75: js::RecordLoopEdge(JSContext*, unsigned int&) (in ./js)
==8341==    by 0x1FFF5B: js::MonitorLoopEdge(JSContext*, unsigned int&) (in ./js)
==8341==    by 0xC5E1E: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (in ./js)
==8341==    by 0xE40D3: js::RunScript(JSContext*, JSScript*, JSStackFrame*) (in ./js)
==8341==    by 0xE4675: js::Execute(JSContext*, JSObject*, JSScript*, JSStackFrame*, unsigned int, js::Value*) (in ./js)
==8341==    by 0x22FD8: JS_ExecuteScript (in ./js)
==8341==    by 0x1562B: Process(JSContext*, JSObject*, char*, int) (in ./js)
==8341==    by 0x1651D: ProcessArgs(JSContext*, JSObject*, char**, int) (in ./js)
==8341==    by 0x1665B: Shell(JSContext*, int, char**, char**) (in ./js)
==8341==  Address 0x620b1c0 is not stack'd, malloc'd or (recently) free'd

So there are two invalid writes in tracejit-generated code. Any way to find out where these happen?
(Reporter)

Comment 5

8 years ago
Created attachment 496884 [details]
ASM dump

I used TMFLAGS=full and annotated (with >>>) the lines where the invalid writes happen. Is there anything different about the arguments object of ES5 strict mode functions?
(Reporter)

Updated

8 years ago
Attachment #496884 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 6

8 years ago
From TraceRecoder::newArguments:
---
if (strict) {
    LIns* argsData_ins = w.getObjPrivatizedSlot(argsobj_ins, JSObject::JSSLOT_ARGS_DATA);
    ptrdiff_t slotsOffset = offsetof(ArgumentsData, slots);
    cx->fp()->forEachCanonicalActualArg(BoxArg(this, ArgsSlotsAddress(argsData_ins, slotsOffset)));
}
---
The "invalid writes" disappear when I comment out the last line.
Waldo might know something here.

Updated

8 years ago
Assignee: general → jwalden+bmo
(Reporter)

Comment 8

8 years ago
This script:
---
function f() {
    "use strict";
    eval("");
    var a = 0;
    for (var i = 0; i < 13; i++) {
	a--;
    }
}
f();
---
Asserts with (-j): 
Assertion failure: !argsobj.getPrivate(), at ../jsfun.cpp:272

I think it's related, maybe this one is easier to debug.
Created attachment 498424 [details] [diff] [review]
fix

I happened on this bug today. The problem is in TraceRecorder::newArguments. The code looks like this:
    if (strict) {
        LIns* argsData_ins = w.getObjPrivatizedSlot(...);
        ptrdiff_t slotsOffset = offsetof(ArgumentsData, slots);
        cx->fp()->forEachCanonicalActualArg(BoxArg(this, ArgsSlotsAddress(argsData_ins,
                                                                          slotsOffset)));
    }

The problem is that slotsOffset is an offset, but the ArgsSlotsAddress constructor expects it to be a slot index, so it multiplies by sizeof(Value). I just removed the multiply here. I can imagine other ways of fixing it though.

The problem ought to show up in the arguments/strict-args.js test. However, the loop wasn't going high enough to trigger the tracer. It looks like there are a lot of other tests that only go up to 5. We really need to fix these.
Assignee: jwalden+bmo → wmccloskey
Status: NEW → ASSIGNED
Attachment #498424 - Flags: review?

Updated

8 years ago
blocking2.0: --- → ?
OS: Mac OS X → Windows 7
Whiteboard: [sg:critical?]
(Reporter)

Comment 10

8 years ago
With that patch, the script in comment 8 still asserts; filed bug 620335.
OS: Windows 7 → All
Attachment #498424 - Flags: review? → review?(nnethercote)
Comment on attachment 498424 [details] [diff] [review]
fix

There are several XyzSlotsAddress structs, and the constructor for each one takes a slot number, so your patch introduces an inconsistency.  Probably the easiest fix is to rename ArgsSlotsAddress as ArgsSlotOffsetAddress.  r=me with that.
Attachment #498424 - Flags: review?(nnethercote) → review+
It might be good to add jitstats asserts to that test as well, so if we ever reenable them it'll fail if not traced...
(Reporter)

Updated

8 years ago
Severity: normal → critical
Whiteboard: [sg:critical?] → [sg:critical?] [fixed-in-tracemonkey]
blocking2.0: ? → beta9+
http://hg.mozilla.org/mozilla-central/rev/563a9221c6c5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.