Closed
Bug 338709
Opened 18 years ago
Closed 18 years ago
All ReadOnly properties can be overwritten by using Object and try..throw..catch
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: Seno.Aiko, Assigned: mrbkap)
References
Details
(Keywords: testcase, verified1.8.0.5, verified1.8.1)
Attachments
(3 files, 1 obsolete file)
770 bytes,
text/html
|
Details | |
2.50 KB,
patch
|
mrbkap
:
review+
brendan
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.5+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.0.5+
|
Details | Diff | Splinter Review |
With a little bit of trickery, all ReadOnly properties can be set to any value: 1. Set Object to a function returning the object whose property you want to write to. 2. throw the new value 3. catch the new value and let the catch clause identifier be the property name you want to overwrite. As far as I can tell this is merely a trivial problem for the JS shell. I don't know whether it's possible to overwrite vital data in the browser this way, though.
Assignee | ||
Comment 2•18 years ago
|
||
Hmm, this might actually be per-spec. There's no wording in the spec to tell the implementation here to find the original Object constructor or prototype or anything like that. On the other hand, if we wanted to do that, we'd need a new way for the exception-emitting code to lookup the original Object constructor. Currently, it emits |name "Object"; pushobj; newinit| which will find the overridden Object. Do we need a new bytecode?
OS: Windows Server 2003 → All
Hardware: PC → All
The wording regarding ReadOnly properties seems unambigous, see section 4.2: "when the ReadOnly attribute for a property is set to true, any attempt by executed ECMAScript code to change the value of the property has no effect." I believe the right fix would be the following: If the "new Object" returned an object with has a ReadOnly property with the same name as the catch identifier, don't put the thrown value on that object and execute the catch block. The catch block would not have access to the thrown value but who cares? I believe this is what IE and Opera do. Alternatively, simply make Object ReadOnly. That's not compatible with the spec but may be more sensible. Is there a good reason to overwrite Object anyway?
Assignee | ||
Comment 4•18 years ago
|
||
(In reply to comment #3) > I believe the right fix would be the following: If the "new Object" returned an > object with has a ReadOnly property with the same name as the catch identifier, > don't put the thrown value on that object and execute the catch block. The I thought about that, but this loses the returned value. Further thinking led me to the same conclusion "who cares". I say the answer to that question is "nobody". Similarly, this patch changes the behavior of |try { throw 42 } catch(__parent__) { print(__parent__) }| I think that anybody depending on the old behavior was broken anyway... > Alternatively, simply make Object ReadOnly. That's not compatible with the spec > but may be more sensible. Is there a good reason to overwrite Object anyway? We've been considering this, and it's currently proposed for ECMAScript v4. Brendan considers this too much of a compatibility hit for Firefox 2, however.
Assignee | ||
Comment 5•18 years ago
|
||
Comment 6•18 years ago
|
||
Comment on attachment 222788 [details] [diff] [review] Minimal fix > /* Define obj[id] to contain rval and to be permanent. */ > SAVE_SP_AND_PC(fp); >- ok = OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, >- JSPROP_PERMANENT, NULL); The "Define obj[id]..." comment seems lost now, or a bit premature at any rate. >+ >+ /* >+ * It's possible for an evil script to substitute a random object >+ * for the new object. Check to make sure that we don't override a >+ * readonly property with the below OBJ_DEFINE_PROPERTY. >+ */ >+ ok = OBJ_GET_ATTRIBUTES(cx, obj, id, NULL, &attrs); > if (!ok) > goto out; >+ if (!(attrs & JSPROP_READONLY)) { This is good, it mimics ECMA-262 [[CanPut]] testingn from [[Put]]. It's conservative, in that if someone forgot to use JSPROP_PERMANENT as well, we would not overwrite, even though readonly without permanent is futile in light of delete. Anyway, this is the "new opcode" -- a fixed version of the old JSOP_INITCATCHVAR, which was always buggy in light of ECMA-262 Edition 3's inconsistent choice to use the current value of 'Object' when constructing the scope for the catch block (see 12.14, semantics for the Catch production). For Edition 4 we will make catch blocks use lexical scope. I'll probably do this for JS1.7 if there is time. r=me with comment nit picked, if it can be picked cleanly. /be
Attachment #222788 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 7•18 years ago
|
||
Fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 8•18 years ago
|
||
Nominating for the branches. This is a small fix and given that the severity of the problem here is unknown, it seems better to be safe than sorry.
Attachment #222788 -
Attachment is obsolete: true
Attachment #222948 -
Flags: review+
Attachment #222948 -
Flags: approval1.8.0.5?
Attachment #222948 -
Flags: approval-branch-1.8.1?(brendan)
Comment 9•18 years ago
|
||
Comment on attachment 222948 [details] [diff] [review] What I checked in Agreed -- this should go into the 1.8.0 branch too. /be
Attachment #222948 -
Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
Reporter | ||
Comment 10•18 years ago
|
||
In the browser the testcase still sets document.documentElement, which usually results in a "setting a property that has only a getter" error. Is this behaviour expected and harmless?
Assignee | ||
Updated•18 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•18 years ago
|
||
With the current trunk, the testcase fails to work at all (thanks to the early |goto out| in js_Interpret), but this patch will fix it anyway (since we shouldn't blow over permanent properties or properties that already have getters or setters).
Attachment #224473 -
Flags: review?(brendan)
Comment 12•18 years ago
|
||
Comment on attachment 224473 [details] [diff] [review] Fix that too Great -- we'll get this into the js1.7a landing for the 1.8 branch, but could you also file a bug for after that landing, to switch to an ES4-like lexical block for catch vars? Thanks, /be
Attachment #224473 -
Flags: review?(brendan)
Attachment #224473 -
Flags: review+
Attachment #224473 -
Flags: approval1.8.0.5?
Assignee | ||
Comment 13•18 years ago
|
||
Fix checked into trunk.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 14•18 years ago
|
||
Comment on attachment 222948 [details] [diff] [review] What I checked in approved for 1.8.0 branch, a=dveditz for drivers
Attachment #222948 -
Flags: approval1.8.0.5? → approval1.8.0.5+
Comment 15•18 years ago
|
||
Comment on attachment 224473 [details] [diff] [review] Fix that too approved for 1.8.0 branch, a=dveditz for drivers Please land these on the 1.8 branch also
Attachment #224473 -
Flags: approval1.8.0.5? → approval1.8.0.5+
Assignee | ||
Comment 16•18 years ago
|
||
Fix checked into the 1.8 branches.
Keywords: fixed1.8.0.5,
fixed1.8.1
Comment 17•18 years ago
|
||
Is it intentional that the exception not be caught at all in: Object = function () { return Math }; try { throw 1990; } catch (LN2) { }
Assignee | ||
Comment 18•18 years ago
|
||
(In reply to comment #17) > Is it intentional that the exception not be caught at all in: I don't think so. Could you file a bug on this?
Comment 19•18 years ago
|
||
Checking in regress-338709.js; /cvsroot/mozilla/js/tests/js1_5/Object/regress-338709.js,v <-- regress-338709.js initial revision: 1.1 passes in 1.8.0.5, 1.8.1 winxp from today but fails due to uncaught exception in 1.9a1 filed Bug 341828
Flags: in-testsuite+
Comment 20•18 years ago
|
||
Verified fixed on mac 1.5.0. branch build id 2006062008 and today's 2.0 branch build. The testcase does not display on trunk.
Status: RESOLVED → VERIFIED
Updated•18 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•