Closed
Bug 566136
Opened 14 years ago
Closed 14 years ago
Assertion failure: isNative() after global.__proto__ = []
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: igor, Assigned: igor)
References
Details
(4 keywords, Whiteboard: [sg:critical?] fixed-in-tracemonkey [critsmash:patch])
Attachments
(2 files, 1 obsolete file)
2.53 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
653 bytes,
patch
|
brendan
:
review+
christian
:
approval1.9.2.7+
christian
:
approval1.9.1.11+
|
Details | Diff | Splinter Review |
The following scripts gives an assert in debug builds: ~/m/tm/js/src> cat ~/s/x.js this.__proto__ = []; print(length); ~/m/tm/js/src> ~/build/js/tdbg/js ~/s/x.js Assertion failure: isNative(), at /home/igor/m/tm/js/src/jsscope.h:549 Aborted
Assignee | ||
Comment 1•14 years ago
|
||
The bug exists since the moment of introduction of the property cache so I nominate it for branches down to 190.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Flags: blocking1.9.0.19?
Assignee | ||
Comment 2•14 years ago
|
||
The patch adds the missing isNative check
Assignee: general → igor
Attachment #445612 -
Flags: review?(brendan)
Comment 3•14 years ago
|
||
Comment on attachment 445612 [details] [diff] [review] v1 Should we make fill test pobj->isNative() instead? I.e., are there other paths that could lead here? /be
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3) > (From update of attachment 445612 [details] [diff] [review]) > Should we make fill test pobj->isNative() instead? I.e., are there other paths > that could lead here? That was really good point. js_FindPropertyHelper also suffers from this bug as the following test case shows: this.__proto__ = []; function f() { eval('Math'); length = 2; } f(); On the other hand AFAICS all other users of LookupProperty calls consistently check for native objects before casting prop to JSScopeProperty. Thus I prefer not to penalize the fill method and rather add the spot checks to the two bad users. But we should definitely do some fuzzing. Gary, could you run a fuzzer after doing an assignments like this.__proto__ = [] or this.__proto__ = <xml/> ?
Assignee | ||
Comment 5•14 years ago
|
||
v2 adds another spot check for non-native prototypes of the global objects.
Attachment #445612 -
Attachment is obsolete: true
Attachment #445617 -
Flags: review?(brendan)
Attachment #445612 -
Flags: review?(brendan)
Comment 6•14 years ago
|
||
js::PropertyCache::fill needs to be cold path or the cache isn't working. I suppose we can rely on the assertion plus fuzzing. I will wait till tomorrow to review in case fuzzing reveals other paths (in which case, or even so: let's test in fill and not worry until profile data shows it slows down benchmarks). /be
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6) > let's > test in fill and not worry until profile data shows it slows down benchmarks). Doing the check in the fill is not enough. Consider that js_FindIdentifierBase is calling JS_UNLOCK_OBJ on the obj assuming that pobj is native. So we really have to monitor all the lookup users. For this reason I still prefer to do the check in the callers, not in the fill.
Comment 8•14 years ago
|
||
You're right that js_FindIdentifierBase needs to follow the rules -- when did it stop? Did it used to loop over only js_IsCacheableNonGlobalScope(obj) and more recently also the global object was included in the loop? It's huge to be able to count on native-only proto chain, but the general case of obj->lookupProperty requires obj->dropProperty after (until we get rid of that as proposed elsewhere). Breaking the general rule proliferates if-not-native tests. /be
Comment 9•14 years ago
|
||
I'm really asking whether it makes sense to unroll any final iteration of the loops in js_FindPropertyHelper and js_FindIdentifierBase if the global object is hit, since the cacheable non-global scope chain objects do not require this is-native checking, and indeed seem to share less code with the global object last-iteration case anyway. /be
Comment 10•14 years ago
|
||
(In reply to comment #4) > all other users of LookupProperty calls consistently check for native objects > before casting prop to JSScopeProperty This is where the root problem lies; we'll continue to have problems as long as we keep doing this without including some sort of automatic checks. Could we provide a way of converting that will assert nativeness? struct JSProperty { JSScopeProperty* asScopeProperty(JSObject* obj) const { union { const JSProperty* prop; const JSScopeProperty* sprop; } u; u.prop = this; JS_ASSERT(obj->scope()->hasProperty(u.sprop)); return u.sprop; } }; Or something in the same spirit with more rigor and less strawman-ness?
Comment 11•14 years ago
|
||
We're getting rid of the JSProperty * opaque pointer type from JSObjectOps hooks, instead. See bug 566143. /be
I'd be pretty cool with __proto__ setting on a JSCLASS_IS_GLOBAL_OBJECT thing throwing an error, too. We're going to get rid of settable proto (vs initializable proto) in due course anyway, and it seems like we can take a first stab by blocking some of the more ludicrous uses. :-)
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #12) > I'd be pretty cool with __proto__ setting on a JSCLASS_IS_GLOBAL_OBJECT thing > throwing an error, too. This would not help this bug as a rogue script can set the proto of global.__proto__. The latter is Object.prototype which is a plain object. > We're going to get rid of settable proto That helps but only if there is guarantee that it would be impossible to construct a global object with non-native on the prototype chain.
Assignee | ||
Comment 14•14 years ago
|
||
(In reply to comment #8) > You're right that js_FindIdentifierBase needs to follow the rules -- when did > it stop? Did it used to loop over only js_IsCacheableNonGlobalScope(obj) and > more recently also the global object was included in the loop? This bug is a regression from my patch for the bug 487039. I was too zealous there optimizing the lookup.
Blocks: 487039
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #9) > I'm really asking whether it makes sense to unroll any final iteration of the > loops in js_FindPropertyHelper and js_FindIdentifierBase I have tried, but that makes the patch much bigger and I prefer a spot fixes here to simplify back-porting.
Updated•14 years ago
|
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .5+
blocking2.0: ? → beta1+
Whiteboard: [sg:critical?]
Updated•14 years ago
|
Updated•14 years ago
|
Attachment #445617 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/07756d509077
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
Updated•14 years ago
|
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey [critsmash:patch]
Comment 17•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/07756d509077
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
blocking1.9.2: .5+ → .6+
Assignee | ||
Comment 18•14 years ago
|
||
Compared with the trunk on 192-191 there is no need to fix js_FindIdentifierBase. The latter on those branches does not cache the lookup for globals as code there relies just on gvar optimization. In js_FindPropertyHelper I also have not added an assert that the global should be native. I do not remember if a non-native global was completely disabled there. Brendan, could you review this before before the freeze on Friday?
Attachment #445617 -
Attachment is obsolete: true
Attachment #453882 -
Flags: review?(brendan)
Assignee | ||
Updated•14 years ago
|
Attachment #445617 -
Attachment is obsolete: false
Updated•14 years ago
|
Attachment #453882 -
Flags: review?(brendan)
Attachment #453882 -
Flags: review+
Attachment #453882 -
Flags: approval1.9.2.6?
Attachment #453882 -
Flags: approval1.9.1.11?
Attachment #453882 -
Flags: approval1.9.2.7?
Attachment #453882 -
Flags: approval1.9.2.7+
Attachment #453882 -
Flags: approval1.9.1.11?
Attachment #453882 -
Flags: approval1.9.1.11+
Comment 19•14 years ago
|
||
Comment on attachment 453882 [details] [diff] [review] fix for 192 and 191 a=LegNeato for 1.9.2.7 and 1.9.1.11. Please land this on mozilla-1.9.2 default and mozilla-1.9.1 default asap. Thanks!
Assignee | ||
Comment 20•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1151e590b153 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1243f15ec0a8
Comment 21•14 years ago
|
||
Bob, can you verify this on 1.9.1 and 1.9.2?
Comment 22•14 years ago
|
||
tests in comment 0 and comment 4 do not assert in the shell for 1.9.1, 1.9.2 on mac os x 10.5, linux 32/64 bit or winxp. v 1.9.1, 1.9.2
Keywords: verified1.9.1,
verified1.9.2
Updated•14 years ago
|
Group: core-security
Assignee | ||
Updated•12 years ago
|
Flags: blocking1.9.0.19?
Updated•11 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•