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)

defect

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)

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.
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?
(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.
Attached patch Minimal fix (obsolete) — Splinter Review
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #222788 - Flags: review?(brendan)
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+
Fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
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 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+
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fix that tooSplinter Review
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 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?
Fix checked into trunk.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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 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+
Fix checked into the 1.8 branches.
Is it intentional that the exception not be caught at all in:

Object = function () { return Math };
try 
{ 
  throw 1990; 
}
catch (LN2) 
{
}

(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?
Blocks: 341828
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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: