From: ricow@chromium.org Date: Tue, 25 May 2010 06:25:27 +0000 (+0000) Subject: Fixes issue 712 causing non-configurable accessors to be overwritable by using X-Git-Tag: upstream/4.7.83~21755 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=fb58bc06c6d5efc56663acd9f05b83a807a96322;p=platform%2Fupstream%2Fv8.git Fixes issue 712 causing non-configurable accessors to be overwritable by using Object.defineProperty with empty property descriptor. The issue is fixed by implementing step 5 and 6 from DefineOwnProperty in the specification (ES5 8.12.9). This also fixes a bug in SameValue when used on boolean values (it would priorly return a number - not a boolean). Review URL: http://codereview.chromium.org/2131019 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@4708 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- diff --git a/src/runtime.js b/src/runtime.js index 8e3883f..887436a 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -562,15 +562,14 @@ function SameValue(x, y) { if (IS_NULL_OR_UNDEFINED(x)) return true; if (IS_NUMBER(x)) { if (NUMBER_IS_NAN(x) && NUMBER_IS_NAN(y)) return true; - // x is +0 and y is -0 or vice versa - if (x === 0 && y === 0 && !%_IsSmi(x) && !%_IsSmi(y) && - ((1 / x < 0 && 1 / y > 0) || (1 / x > 0 && 1 / y < 0))) { + // x is +0 and y is -0 or vice versa. + if (x === 0 && y === 0 && (1 / x) != (1 / y)) { return false; } return x == y; } if (IS_STRING(x)) return %StringEquals(x, y); - if (IS_BOOLEAN(x))return %NumberEquals(%ToNumber(x),%ToNumber(y)); + if (IS_BOOLEAN(x)) return y == x; return %_ObjectEquals(x, y); } diff --git a/src/v8natives.js b/src/v8natives.js index 531bd0e..ecb8eee 100644 --- a/src/v8natives.js +++ b/src/v8natives.js @@ -434,6 +434,11 @@ PropertyDescriptor.prototype.isWritable = function() { } +PropertyDescriptor.prototype.hasWritable = function() { + return this.hasWritable_; +} + + PropertyDescriptor.prototype.setConfigurable = function(configurable) { this.configurable_ = configurable; this.hasConfigurable_ = true; @@ -537,6 +542,22 @@ function DefineOwnProperty(obj, p, desc, should_throw) { throw MakeTypeError("define_disallowed", ["defineProperty"]); if (!IS_UNDEFINED(current) && !current.isConfigurable()) { + // Step 5 and 6 + if ((!desc.hasEnumerable() || + SameValue(desc.isEnumerable() && current.isEnumerable())) && + (!desc.hasConfigurable() || + SameValue(desc.isConfigurable(), current.isConfigurable())) && + (!desc.hasWritable() || + SameValue(desc.isWritable(), current.isWritable())) && + (!desc.hasValue() || + SameValue(desc.getValue(), current.getValue())) && + (!desc.hasGetter() || + SameValue(desc.getGet(), current.getGet())) && + (!desc.hasSetter() || + SameValue(desc.getSet(), current.getSet()))) { + return true; + } + // Step 7 if (desc.isConfigurable() || desc.isEnumerable() != current.isEnumerable()) throw MakeTypeError("redefine_disallowed", ["defineProperty"]); @@ -673,8 +694,9 @@ function ObjectCreate(proto, properties) { // ES5 section 15.2.3.6. function ObjectDefineProperty(obj, p, attributes) { if ((!IS_SPEC_OBJECT_OR_NULL(obj) || IS_NULL_OR_UNDEFINED(obj)) && - !IS_UNDETECTABLE(obj)) + !IS_UNDETECTABLE(obj)) { throw MakeTypeError("obj_ctor_property_non_object", ["defineProperty"]); + } var name = ToString(p); var desc = ToPropertyDescriptor(attributes); DefineOwnProperty(obj, name, desc, true); diff --git a/test/mjsunit/object-define-property.js b/test/mjsunit/object-define-property.js index 43b1c7f..ba50767 100644 --- a/test/mjsunit/object-define-property.js +++ b/test/mjsunit/object-define-property.js @@ -53,36 +53,46 @@ try { assertTrue(/called on non-object/.test(e)); } -// Object +// Object. var obj1 = {}; -// Values +// Values. var val1 = 0; var val2 = 0; var val3 = 0; -// Descriptors +function setter1() {val1++; } +function getter1() {return val1; } + +function setter2() {val2++; } +function getter2() {return val2; } + +function setter3() {val3++; } +function getter3() {return val3; } + + +// Descriptors. var emptyDesc = {}; var accessorConfigurable = { - set: function() { val1++; }, - get: function() { return val1; }, + set: setter1, + get: getter1, configurable: true }; var accessorNoConfigurable = { - set: function() { val2++; }, - get: function() { return val2; }, + set: setter2, + get: getter2, configurable: false }; var accessorOnlySet = { - set: function() { val3++; }, + set: setter3, configurable: true }; var accessorOnlyGet = { - get: function() { return val3; }, + get: getter3, configurable: true }; @@ -200,7 +210,7 @@ assertEquals(2, val1); assertEquals(4, val2); assertEquals(4, obj1.bar); -// Define an accessor that has only a setter +// Define an accessor that has only a setter. Object.defineProperty(obj1, "setOnly", accessorOnlySet); desc = Object.getOwnPropertyDescriptor(obj1, "setOnly"); assertTrue(desc.configurable); @@ -212,7 +222,7 @@ assertEquals(desc.get, undefined); assertEquals(1, obj1.setOnly = 1); assertEquals(1, val3); -// Add a getter - should not touch the setter +// Add a getter - should not touch the setter. Object.defineProperty(obj1, "setOnly", accessorOnlyGet); desc = Object.getOwnPropertyDescriptor(obj1, "setOnly"); assertTrue(desc.configurable); @@ -256,7 +266,7 @@ obj1.foobar = 1001; assertEquals(obj1.foobar, 1000); -// Redefine to writable descriptor - now writing to foobar should be allowed +// Redefine to writable descriptor - now writing to foobar should be allowed. Object.defineProperty(obj1, "foobar", dataWritable); desc = Object.getOwnPropertyDescriptor(obj1, "foobar"); assertEquals(obj1.foobar, 3000); @@ -375,7 +385,7 @@ assertEquals(desc.set, undefined); // Redefinition of an accessor defined using __defineGetter__ and -// __defineSetter__ +// __defineSetter__. function get(){return this.x} function set(x){this.x=x}; @@ -442,7 +452,7 @@ assertEquals(1, obj4.bar = 1); assertEquals(5, val1); assertEquals(5, obj4.bar); -// Make sure an error is thrown when trying to access to redefined function +// Make sure an error is thrown when trying to access to redefined function. try { obj4.bar(); assertTrue(false); @@ -453,7 +463,7 @@ try { // Test runtime calls to DefineOrRedefineDataProperty and // DefineOrRedefineAccessorProperty - make sure we don't -// crash +// crash. try { %DefineOrRedefineAccessorProperty(0, 0, 0, 0, 0); } catch (e) { @@ -497,3 +507,210 @@ try { } catch (e) { assertTrue(/illegal access/.test(e)); } + +// Test that all possible differences in step 6 in DefineOwnProperty are +// exercised, i.e., any difference in the given property descriptor and the +// existing properties should not return true, but throw an error if the +// existing configurable property is false. + +var obj5 = {}; +// Enumerable will default to false. +Object.defineProperty(obj5, 'foo', accessorNoConfigurable); +desc = Object.getOwnPropertyDescriptor(obj5, 'foo'); +// First, test that we are actually allowed to set the accessor if all +// values are of the descriptor are the same as the existing one. +Object.defineProperty(obj5, 'foo', accessorNoConfigurable); + +// Different setter. +var descDifferent = { + configurable:false, + enumerable:false, + set: setter1, + get: getter2 +}; + +try { + Object.defineProperty(obj5, 'foo', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + +// Different getter. +descDifferent = { + configurable:false, + enumerable:false, + set: setter2, + get: getter1 +}; + +try { + Object.defineProperty(obj5, 'foo', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + +// Different enumerable. +descDifferent = { + configurable:false, + enumerable:true, + set: setter2, + get: getter2 +}; + +try { + Object.defineProperty(obj5, 'foo', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + +// Different configurable. +descDifferent = { + configurable:false, + enumerable:true, + set: setter2, + get: getter2 +}; + +try { + Object.defineProperty(obj5, 'foo', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + +// No difference. +descDifferent = { + configurable:false, + enumerable:false, + set: setter2, + get: getter2 +}; +// Make sure we can still redefine if all properties are the same. +Object.defineProperty(obj5, 'foo', descDifferent); + +// Make sure that obj5 still holds the original values. +desc = Object.getOwnPropertyDescriptor(obj5, 'foo'); +assertEquals(desc.get, getter2); +assertEquals(desc.set, setter2); +assertFalse(desc.enumerable); +assertFalse(desc.configurable); + + +// Also exercise step 6 on data property, writable and enumerable +// defaults to false. +Object.defineProperty(obj5, 'bar', dataNoConfigurable); + +// Test that redefinition with the same property descriptor is possible +Object.defineProperty(obj5, 'bar', dataNoConfigurable); + +// Different value. +descDifferent = { + configurable:false, + enumerable:false, + writable: false, + value: 1999 +}; + +try { + Object.defineProperty(obj5, 'bar', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + +// Different writable. +descDifferent = { + configurable:false, + enumerable:false, + writable: true, + value: 2000 +}; + +try { + Object.defineProperty(obj5, 'bar', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + + +// Different enumerable. +descDifferent = { + configurable:false, + enumerable:true , + writable:false, + value: 2000 +}; + +try { + Object.defineProperty(obj5, 'bar', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + + +// Different configurable. +descDifferent = { + configurable:true, + enumerable:false, + writable:false, + value: 2000 +}; + +try { + Object.defineProperty(obj5, 'bar', descDifferent); + assertTrue(false); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + +// No difference. +descDifferent = { + configurable:false, + enumerable:false, + writable:false, + value:2000 +}; +// Make sure we can still redefine if all properties are the same. +Object.defineProperty(obj5, 'bar', descDifferent); + +// Make sure that obj5 still holds the original values. +desc = Object.getOwnPropertyDescriptor(obj5, 'bar'); +assertEquals(desc.value, 2000); +assertFalse(desc.writable); +assertFalse(desc.enumerable); +assertFalse(desc.configurable); + + +// Make sure that we can't overwrite +0 with -0 and vice versa. +var descMinusZero = {value: -0, configurable: false}; +var descPlusZero = {value: +0, configurable: false}; + +Object.defineProperty(obj5, 'minuszero', descMinusZero); + +// Make sure we can redefine with -0. +Object.defineProperty(obj5, 'minuszero', descMinusZero); + +try { + Object.defineProperty(obj5, 'minuszero', descPlusZero); + assertUnreachable(); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} + + +Object.defineProperty(obj5, 'pluszero', descPlusZero); + +// Make sure we can redefine with +0. +Object.defineProperty(obj5, 'pluszero', descPlusZero); + +try { + Object.defineProperty(obj5, 'pluszero', descMinusZero); + assertUnreachable(); +} catch (e) { + assertTrue(/Cannot redefine property/.test(e)); +} diff --git a/test/mjsunit/regress/regress-712.js b/test/mjsunit/regress/regress-712.js new file mode 100644 index 0000000..b26b94a --- /dev/null +++ b/test/mjsunit/regress/regress-712.js @@ -0,0 +1,38 @@ +// Copyright 2010 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// This regression test is used to ensure that Object.defineProperty +// can't be called with an empty property descriptor on a non-configurable +// existing property and override the existing property. +// See: http://code.google.com/p/v8/issues/detail?id=712 + +var obj = {}; +Object.defineProperty(obj, "x", { get: function() { return "42"; }, + configurable: false }); +assertEquals(obj.x, "42"); +Object.defineProperty(obj, 'x', {}); +assertEquals(obj.x, "42");