406572 - JSOP_CLOSURE unconditionally replaces properties of the variable object

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Igor Bukanov]

Attachment: test case overwriting window.document

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.

Comment 1

[Igor Bukanov]

The patch from bug 406477 comment 10 includes the fix this bug through code sharing between JSOP_DEFUN and JSOP_CLOSURE.

Comment 2

[Brendan Eich [:brendan]]

Attachment: plain diff of JSOP_{DEFFUN,CLOSURE} code, w/o comments&whitespace

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

Comment 3

[Igor Bukanov]

(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.

Comment 4

[Igor Bukanov]

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.

Comment 5

[Igor Bukanov]

The bug was fixed as a part of a check in for bug 406477.

Comment 6

[Igor Bukanov]

Attachment: patch for branch

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.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 296606 [details] [diff] [review]
patch for branch

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 8

[Samuel Sidler (old account; do not CC)]

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.

Comment 9

[Igor Bukanov]

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

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 11

[Alexander Sack]

Comment on attachment 296606 [details] [diff] [review]
patch for branch

a=asac for 1.8.0.15

Comment 12

[Reed Loden [:reed]]

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

Comment 13

[Bob Clary [:bc] (inactive)]

verified linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-406572.js,v  <--  regress-406572.js
initial revision: 1.1

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e31a0fc42be