Closed Bug 406572 Opened 12 years ago Closed 12 years ago

JSOP_CLOSURE unconditionally replaces properties of the variable object

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Keywords: fixed1.8.0.15, testcase, verified1.8.1.12, Whiteboard: [sg:high?])

Attachments

(3 files)

Currently JSOP_CLOSURE unconditionally calls OBJ_DEFINE_PROPERTY on the variable object without checking that the property is not ReadOnly or DontDelete. It allows to replace const properties of the global object. The attached example shows that altering window.document.  

Compare that with JSOP_DEFFUN case which since landing the bug 21618 explicitly prevents such overwrites.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
The patch from bug 406477 comment 10 includes the fix this bug through code sharing between JSOP_DEFUN and JSOP_CLOSURE.
These are so close, I like the idea of sharing code, as you did in the other bug's patch. Could we share more? We do not want js_CheckRedeclaration if JSOP_DEFFUN. We could avoid js_GetScopeChain for that op too. Thoughts?

/be
(In reply to comment #2)
> We do not want js_CheckRedeclaration if JSOP_DEFFUN.

It is exactly the js_CheckRedeclaration call in JSOP_DEFFUN that prevents this bug to happen there so adding it for JSOP_CLOSURE should addresses the issue as well. Then JSOP_CLOSURE/JSOP_DEFFUN would be essentially the same except for the scope chain initialization since it is known in JSOP_DEFFUN that the block chain is null. Plus there is a special support for the eval in DEFFUN and its absence in CLOSURE. 

The latter exposes yet another bug since it means that eval("if (1) function f() {}") creates f as a permanent property and the patch from bug 406477 comment 10 solves this as well.
To Damon Sicore:

The patch bug 406477 comment 10 contains includes a fix for this bug under the carpet. Since the bug exists on 1.8.1 and its consequences can be rather bad, it would be nice to fix this under a cover bug on the trunk. Otherwise a diff of targeted fix for the trunk would point to the problem nicely.
The bug was fixed as a part of a check in for bug 406477.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Attached patch patch for branchSplinter Review
For minimality the patch just protect OBJ_DEFINE_PROPERTY with js_CheckRedeclaration in JSOP_CLOSURE in the same way JSOP_DEFFUN does. I also removed that #if 0 code since it referes to attrs which the patch modifies and since the code is removed on the trunk.
Attachment #296606 - Flags: review?(brendan)
Attachment #296606 - Flags: review?(brendan)
Attachment #296606 - Flags: review+
Attachment #296606 - Flags: approval1.8.1.12?
Comment on attachment 296606 [details] [diff] [review]
patch for branch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #296606 - Flags: approval1.8.1.12? → approval1.8.1.12+
QA: To verify, ensure that there are only two lines of content (both with only {}) when viewing the attached testcase.

Currently, on branch, you will see "function document() {return 1;}" as a third line. This should not be the case and is not the case on trunk.
Keywords: testcase
I checked in the patch from comment 6 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.94; previous revision: 3.181.2.93
done
Keywords: fixed1.8.1.12
Verified for branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre.
Whiteboard: [sg:?]
Group: security
Whiteboard: [sg:?] → [sg:high?]
Comment on attachment 296606 [details] [diff] [review]
patch for branch

a=asac for 1.8.0.15
Attachment #296606 - Flags: approval1.8.0.15+
Flags: blocking1.8.0.15+
Keywords: checkin-needed
MOZILLA_1_8_0_BRANCH:

Checking in js/src/jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.35; previous revision: 3.181.2.17.2.34
done
verified linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-406572.js,v  <--  regress-406572.js
initial revision: 1.1
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.