Closed
Bug 563210
Opened 14 years ago
Closed 14 years ago
"Assertion failure: isDenseArrayMinLenCapOk()," with gc, gczeal, Array, defineGetter
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: gkw, Assigned: n.nethercote)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [sg:nse], fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
8.16 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
try { __defineGetter__("x", gc) } catch (e) {} gczeal(1) print(x)(Array(-8)) asserts js debug shell on TM tip without -j at Assertion failure: isDenseArrayMinLenCapOk(), at ../jsobjinlines.h:195 Setting s-s because this involves gc and gczeal.
Reporter | ||
Comment 1•14 years ago
|
||
autoBisect shows this is probably related to bug 562571: The first bad revision is: changeset: 41517:990192b0e052 user: Nicholas Nethercote date: Thu Apr 29 20:22:33 2010 -0700 summary: Bug 562571 - TM: don't have two bounds checks for array getelem. r=brendan.
Assignee | ||
Comment 2•14 years ago
|
||
Yeah, bug 562571 is definitely responsible, that assertion was added in that revision. I'll take a look tomorrow.
Updated•14 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 3•14 years ago
|
||
It's similar to a problem I saw when developing the patch for bug 562571: getDenseArrayCapacity() is called and the MINLENCAP and LENGTH slots are still set to JSVAL_VOID (the numeric value of which is 22). In other words, it's a question of initialisation. Here's the stack trace: #0 in __kernel_vsyscall () #1 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42 #2 in JS_Assert at ../jsutil.cpp:79 #3 in JSObject::getDenseArrayCapacity at ../jsobjinlines.h:196 #4 in array_trace at ../jsarray.cpp:1239 #5 in JS_TraceChildren at ../jsgc.cpp:1833 #6 in js_CallGCMarker at ../jsgc.cpp:2109 #7 in JS_CallTracer at ../jsapi.cpp:1882 #8 in js_TraceStackFrame at ../jsgc.cpp:2299 #9 in TraceFrameChain at ../jsgc.cpp:2344 #10 in js_TraceContext at ../jsgc.cpp:2368 #11 in js_TraceRuntime at ../jsgc.cpp:2432 #12 in GC at ../jsgc.cpp:3007 #13 in GCUntilDone at ../jsgc.cpp:3188 #14 in js_GC at ../jsgc.cpp:3518 #15 in LastDitchGC at ../jsgc.cpp:1429 #16 in RefillFinalizableFreeList at ../jsgc.cpp:1453 #17 in js_NewFinalizableGCThing at ../jsgc.cpp:1561 #18 in js_NewGCFunction at ../jsgc.h:298 #19 in NewObjectWithGivenProto at ../jsobjinlines.h:505 #20 in NewObject at ../jsobjinlines.h:588 #21 in js_NewFunction at ../jsfun.cpp:2400 #22 in js_DefineFunction at ../jsfun.cpp:2554 #23 in js_InitExceptionClasses at ../jsexn.cpp:1026 #24 in js_GetClassObject at ../jsobj.cpp:3770 #25 in js_FindClassObject at ../jsobj.cpp:3835 #26 in js_GetClassPrototype at ../jsobj.cpp:5977 #27 in js_ErrorToException at ../jsexn.cpp:1157 #28 in ReportError at ../jscntxt.cpp:1321 #29 in js_ReportErrorNumberVA at ../jscntxt.cpp:1676 #30 in JS_ReportErrorNumber at ../jsapi.cpp:5395 #31 in ValueIsLength at ../jsarray.cpp:228 #32 in js_Array at ../jsarray.cpp:3350 #33 in js_Invoke at ../jsinterp.cpp:834 #34 in js_Interpret at ../jsops.cpp:2257 #35 in js_Execute at ../jsinterp.cpp:1103 #36 in JS_ExecuteScript at ../jsapi.cpp:4818 #37 in Process at ../../shell/js.cpp:449 #38 in ProcessArgs at ../../shell/js.cpp:863 #39 in main at ../../shell/js.cpp:5045 Looks we try to construct the Array(-8) which causes an exception to be thrown due to the negative size, which somehow causes a GC during which we hit an array that hasn't had its MINLENCAP/LENGTH slots initialised yet. Brendan, do you have any idea about this? FWIW, in this particular case nothing bad will happen, because capacity is set correctly for the getDenseArrayCapacity() call, but it's a concern that this has been hit.
Comment 4•14 years ago
|
||
There's nothing mysterious. Your analysis is accurate. Built-in classes are initialized lazily in many embeddings, including the browser and shell. The exception classes being initialized require allocation of new gc-things, gczeal runs the GC, and finds the new array object via the stack, before it has been processed by InitArrayObject called from js_Array. Nested GC can happen from many internal APIs, including JS_ReportErrorNumber from ValueIsLength. The array needs initializing before this point, at least of its fixed fields. /be
Assignee | ||
Comment 5•14 years ago
|
||
This patch just adds an extra initialisation, which feels very hacky to me but Brendan said on IRC that it was reasonable. BTW, Jesse, I don't think this is a security bug, the assertion is a bit aggressive, ie. the slots in question are checked before they are ever used.
Comment 6•14 years ago
|
||
Comment on attachment 443033 [details] [diff] [review] patch We have a custom array constructor for on-trace don't we? Is that one affected?
Updated•14 years ago
|
Group: core-security
Whiteboard: [sg:critical?] → [sg:nse]
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 7•14 years ago
|
||
Comment on attachment 443033 [details] [diff] [review] patch A one-line comment such as "Pre-initialize in case of GC nesting here" would be good. /be
Attachment #443033 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 8•14 years ago
|
||
It's mixed metaphor time: I decided it's better to bend in the wind than play whack-a-mole with initialisation. This patch: - Removes JSCLASS_CONSTRUCT_PROTOTYPE from js_ArrayClass, it's no longer necessary. - Gives isDenseArrayMinLenCapOk() an argument controlling how strict it should be with its checking (defaults to strict checking), and makes it public so it can be called within jstracer.cpp when the MINLENCAP slot is accessed directly. - Adds the failing test case to js/src/tests/js1_8_5/regress, please let me know if this is not the right place for it.
Attachment #443033 -
Attachment is obsolete: true
Attachment #443224 -
Flags: review?(brendan)
Comment 9•14 years ago
|
||
Comment on attachment 443224 [details] [diff] [review] patch v2 >+ * friend. There is a function isDenseArrayMinLenCapOk() which checks that >+ * it is set correctly, it should be put in an assertion at use points. How about "The function isDenseArrayMinLenCapOk() checks that it is set correctly; it should be put in an assertion at use points" for passive voice and comma splice avoidance. >@@ -110,23 +110,28 @@ JSObject::setPrimitiveThis(jsval pthis) > fslots[JSSLOT_PRIMITIVE_THIS] = pthis; > } > > inline void JSObject::staticAssertArrayLengthIsInPrivateSlot() > { > JS_STATIC_ASSERT(JSSLOT_ARRAY_LENGTH == JSSLOT_PRIVATE); > } > >+inline bool JSObject::isDenseArrayMinLenCapOk(bool strictWithLength) const These two at least need line breaks after their return types. > { > JS_ASSERT(isDenseArray()); > uint32 length = uncheckedGetArrayLength(); > uint32 capacity = uncheckedGetDenseArrayCapacity(); > uint32 minLenCap = uint32(fslots[JSSLOT_DENSE_ARRAY_MINLENCAP]); >- return minLenCap == JS_MIN(length, capacity); >+ // This function can be called while the LENGTH and MINLENCAP slots are >+ // still set to JSVAL_VOID and there are no dslots (ie. the capacity is >+ // zero). If 'strictWithLength' is false we allow this. Nit: blank line before comment taking 1+ lines. Also, "With" has bad meaning in JS -- suggest "About" instead. >+ JS_ASSERT(isDenseArrayMinLenCapOk(/*strictWithLength*/false)); Suggest "/* strictWithLength = */ false" -- let it breathe. r=me w/ nits picked. /be
Updated•14 years ago
|
Attachment #443224 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 10•14 years ago
|
||
Pushed with nits fixed: http://hg.mozilla.org/tracemonkey/rev/bd32a5d4c86d
Whiteboard: [sg:nse] → [sg:nse], fixed-in-tracemonkey
Comment 11•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bd32a5d4c86d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
blocking2.0: ? → betaN+
Updated•14 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Comment 12•11 years ago
|
||
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-563210.js.
Flags: in-testsuite+
Reporter | ||
Comment 13•11 years ago
|
||
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•