From 5ba1304d60d8c58746bf91e549608090778239a1 Mon Sep 17 00:00:00 2001 From: "mvstanton@chromium.org" Date: Fri, 29 Nov 2013 15:22:16 +0000 Subject: [PATCH] Array builtins need to be prevented from changing frozen objects, and changing structure on sealed objects. BUG=299979 LOG=Y R=verwaest@chromium.org Review URL: https://codereview.chromium.org/80623002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@18164 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/stub-cache-arm.cc | 12 ++++-- src/array.js | 54 ++++++++++++++++++++---- src/builtins.cc | 1 + src/ia32/stub-cache-ia32.cc | 12 ++++-- src/messages.js | 2 + src/mips/stub-cache-mips.cc | 12 ++++-- src/objects-printer.cc | 5 +++ src/x64/stub-cache-x64.cc | 20 ++++----- test/mjsunit/object-freeze.js | 23 ++++++++++ test/mjsunit/object-seal.js | 76 ++++++++++++++++++++++++++++++++++ test/mjsunit/regress/regress-2711.js | 4 +- test/mjsunit/regress/regress-299979.js | 34 +++++++++++++++ 12 files changed, 222 insertions(+), 33 deletions(-) create mode 100644 test/mjsunit/regress/regress-299979.js diff --git a/src/arm/stub-cache-arm.cc b/src/arm/stub-cache-arm.cc index 8f411a4..60c28da 100644 --- a/src/arm/stub-cache-arm.cc +++ b/src/arm/stub-cache-arm.cc @@ -1611,10 +1611,12 @@ Handle CallStubCompiler::CompileArrayPushCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } @@ -1854,10 +1856,12 @@ Handle CallStubCompiler::CompileArrayPopCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } diff --git a/src/array.js b/src/array.js index e98d7f5..26bf728 100644 --- a/src/array.js +++ b/src/array.js @@ -425,6 +425,11 @@ function ArrayPop() { return; } + if ($Object.isSealed(this)) { + throw MakeTypeError("array_functions_change_sealed", + ["Array.prototype.pop"]); + } + if (%IsObserved(this)) return ObservedArrayPop.call(this, n); @@ -462,11 +467,16 @@ function ArrayPush() { ["Array.prototype.push"]); } + var n = TO_UINT32(this.length); + var m = %_ArgumentsLength(); + if (m > 0 && $Object.isSealed(this)) { + throw MakeTypeError("array_functions_change_sealed", + ["Array.prototype.push"]); + } + if (%IsObserved(this)) return ObservedArrayPush.apply(this, arguments); - var n = TO_UINT32(this.length); - var m = %_ArgumentsLength(); for (var i = 0; i < m; i++) { this[i+n] = %_Arguments(i); } @@ -604,6 +614,11 @@ function ArrayShift() { return; } + if ($Object.isSealed(this)) { + throw MakeTypeError("array_functions_change_sealed", + ["Array.prototype.shift"]); + } + if (%IsObserved(this)) return ObservedArrayShift.call(this, len); @@ -645,15 +660,32 @@ function ArrayUnshift(arg1) { // length == 1 ["Array.prototype.unshift"]); } - if (%IsObserved(this)) - return ObservedArrayUnshift.apply(this, arguments); - var len = TO_UINT32(this.length); var num_arguments = %_ArgumentsLength(); + var is_sealed = $Object.isSealed(this); - if (IS_ARRAY(this)) { + if (num_arguments > 0 && is_sealed) { + throw MakeTypeError("array_functions_change_sealed", + ["Array.prototype.unshift"]); + } + + if (%IsObserved(this)) + return ObservedArrayUnshift.apply(this, arguments); + + if (IS_ARRAY(this) && !is_sealed) { SmartMove(this, 0, 0, len, num_arguments); } else { + if (num_arguments == 0 && $Object.isFrozen(this)) { + // In the zero argument case, values from the prototype come into the + // object. This can't be allowed on frozen arrays. + for (var i = 0; i < len; i++) { + if (!this.hasOwnProperty(i) && !IS_UNDEFINED(this[i])) { + throw MakeTypeError("array_functions_on_frozen", + ["Array.prototype.shift"]); + } + } + } + SimpleMove(this, 0, 0, len, num_arguments); } @@ -663,7 +695,7 @@ function ArrayUnshift(arg1) { // length == 1 this.length = len + num_arguments; - return len + num_arguments; + return this.length; } @@ -802,6 +834,14 @@ function ArraySplice(start, delete_count) { deleted_elements.length = del_count; var num_elements_to_add = num_arguments > 2 ? num_arguments - 2 : 0; + if (del_count != num_elements_to_add && $Object.isSealed(this)) { + throw MakeTypeError("array_functions_change_sealed", + ["Array.prototype.splice"]); + } else if (del_count > 0 && $Object.isFrozen(this)) { + throw MakeTypeError("array_functions_on_frozen", + ["Array.prototype.splice"]); + } + var use_simple_splice = true; if (IS_ARRAY(this) && num_elements_to_add !== del_count) { diff --git a/src/builtins.cc b/src/builtins.cc index 1b62223..19e8336 100644 --- a/src/builtins.cc +++ b/src/builtins.cc @@ -307,6 +307,7 @@ static inline MaybeObject* EnsureJSArrayWithWritableFastElements( if (!receiver->IsJSArray()) return NULL; JSArray* array = JSArray::cast(receiver); if (array->map()->is_observed()) return NULL; + if (!array->map()->is_extensible()) return NULL; HeapObject* elms = array->elements(); Map* map = elms->map(); if (map == heap->fixed_array_map()) { diff --git a/src/ia32/stub-cache-ia32.cc b/src/ia32/stub-cache-ia32.cc index 46920fb..5c81b59 100644 --- a/src/ia32/stub-cache-ia32.cc +++ b/src/ia32/stub-cache-ia32.cc @@ -1707,10 +1707,12 @@ Handle CallStubCompiler::CompileArrayPushCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } @@ -1950,10 +1952,12 @@ Handle CallStubCompiler::CompileArrayPopCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } diff --git a/src/messages.js b/src/messages.js index 085b4d8..c709672 100644 --- a/src/messages.js +++ b/src/messages.js @@ -111,6 +111,8 @@ var kMessages = { constructor_not_function: ["Constructor ", "%0", " requires 'new'"], not_a_promise: ["%0", "is not a promise"], promise_cyclic: ["Chaining cycle detected for promise", "%0"], + array_functions_on_frozen: ["Cannot modify frozen array elements"], + array_functions_change_sealed: ["Cannot add/remove sealed array elements"], // RangeError invalid_array_length: ["Invalid array length"], invalid_array_buffer_length: ["Invalid array buffer length"], diff --git a/src/mips/stub-cache-mips.cc b/src/mips/stub-cache-mips.cc index 808d072..ea1c9a5 100644 --- a/src/mips/stub-cache-mips.cc +++ b/src/mips/stub-cache-mips.cc @@ -1597,10 +1597,12 @@ Handle CallStubCompiler::CompileArrayPushCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } @@ -1839,10 +1841,12 @@ Handle CallStubCompiler::CompileArrayPopCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } diff --git a/src/objects-printer.cc b/src/objects-printer.cc index 512f532..381c9aa 100644 --- a/src/objects-printer.cc +++ b/src/objects-printer.cc @@ -556,6 +556,11 @@ void Map::MapPrint(FILE* out) { if (is_access_check_needed()) { PrintF(out, " - access_check_needed\n"); } + if (is_frozen()) { + PrintF(out, " - frozen\n"); + } else if (!is_extensible()) { + PrintF(out, " - sealed\n"); + } PrintF(out, " - back pointer: "); GetBackPointer()->ShortPrint(out); PrintF(out, "\n - instance descriptors %s#%i: ", diff --git a/src/x64/stub-cache-x64.cc b/src/x64/stub-cache-x64.cc index 2e536bc..e7bbe27 100644 --- a/src/x64/stub-cache-x64.cc +++ b/src/x64/stub-cache-x64.cc @@ -1637,18 +1637,12 @@ Handle CallStubCompiler::CompileArrayPushCall( Handle function, Handle name, Code::StubType type) { - // ----------- S t a t e ------------- - // -- rcx : name - // -- rsp[0] : return address - // -- rsp[(argc - n) * 8] : arg[n] (zero-based) - // -- ... - // -- rsp[(argc + 1) * 8] : receiver - // ----------------------------------- - - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } @@ -1884,10 +1878,12 @@ Handle CallStubCompiler::CompileArrayPopCall( Handle function, Handle name, Code::StubType type) { - // If object is not an array or is observed, bail out to regular call. + // If object is not an array or is observed or sealed, bail out to regular + // call. if (!object->IsJSArray() || !cell.is_null() || - Handle::cast(object)->map()->is_observed()) { + Handle::cast(object)->map()->is_observed() || + !Handle::cast(object)->map()->is_extensible()) { return Handle::null(); } diff --git a/test/mjsunit/object-freeze.js b/test/mjsunit/object-freeze.js index a0717a1..3b79874 100644 --- a/test/mjsunit/object-freeze.js +++ b/test/mjsunit/object-freeze.js @@ -314,3 +314,26 @@ assertTrue(%HasFastProperties(obj)); Object.freeze(obj); assertTrue(%HasFastProperties(obj)); assertTrue(Object.isFrozen(obj)); + +// Test array built-in functions with freeze. +obj = [1,2,3]; +Object.freeze(obj); +// if frozen implies sealed, then the tests in object-seal.js are mostly +// sufficient. +assertTrue(Object.isSealed(obj)); + +assertDoesNotThrow(function() { obj.push(); }); +assertDoesNotThrow(function() { obj.unshift(); }); +assertDoesNotThrow(function() { obj.splice(0,0); }); +assertTrue(Object.isFrozen(obj)); + +// Verify that an item can't be changed with splice. +assertThrows(function() { obj.splice(0,1,1); }, TypeError); + +// Verify that unshift() with no arguments will fail if it reifies from +// the prototype into the object. +obj = [1,,3]; +obj.__proto__[1] = 1; +assertEquals(1, obj[1]); +Object.freeze(obj); +assertThrows(function() { obj.unshift(); }, TypeError); diff --git a/test/mjsunit/object-seal.js b/test/mjsunit/object-seal.js index f21baed..f31f0b7 100644 --- a/test/mjsunit/object-seal.js +++ b/test/mjsunit/object-seal.js @@ -28,6 +28,7 @@ // Tests the Object.seal and Object.isSealed methods - ES 15.2.3.9 and // ES 15.2.3.12 +// Flags: --allow-natives-syntax --noalways-opt // Test that we throw an error if an object is not passed as argument. var non_objects = new Array(undefined, null, 1, -1, 0, 42.43); @@ -192,3 +193,78 @@ assertFalse(Object.isSealed(obj4)); // Make sure that Object.seal returns the sealed object. var obj4 = {}; assertTrue(obj4 === Object.seal(obj4)); + +// +// Test that built-in array functions can't modify a sealed array. +// +obj = [1, 2, 3]; +var objControl = [4, 5, 6]; + +// Allow these functions to set up monomorphic calls, using custom built-ins. +var push_call = function(a) { a.push(10); return a; } +var pop_call = function(a) { return a.pop(); } +for (var i = 0; i < 3; i++) { + push_call(obj); + pop_call(obj); +} + +Object.seal(obj); +assertThrows(function() { push_call(obj); }, TypeError); +assertThrows(function() { pop_call(obj); }, TypeError); + +// But the control object is fine at these sites. +assertDoesNotThrow(function() { push_call(objControl); }); +assertDoesNotThrow(function() { pop_call(objControl); }); + +assertDoesNotThrow(function() { obj.push(); }); +assertThrows(function() { obj.push(3); }, TypeError); +assertThrows(function() { obj.pop(); }, TypeError); +assertThrows(function() { obj.shift(3); }, TypeError); +assertDoesNotThrow(function() { obj.unshift(); }); +assertThrows(function() { obj.unshift(1); }, TypeError); +assertThrows(function() { obj.splice(0, 0, 100, 101, 102); }, TypeError); +assertDoesNotThrow(function() { obj.splice(0,0); }); + +assertDoesNotThrow(function() { objControl.push(3); }); +assertDoesNotThrow(function() { objControl.pop(); }); +assertDoesNotThrow(function() { objControl.shift(3); }); +assertDoesNotThrow(function() { objControl.unshift(); }); +assertDoesNotThrow(function() { objControl.splice(0, 0, 100, 101, 102); }); + +// Verify that crankshaft still does the right thing. +obj = [1, 2, 3]; + +push_call = function(a) { a.push(1000); return a; } +// Include a call site that doesn't have a custom built-in. +var shift_call = function(a) { a.shift(1000); return a; } +for (var i = 0; i < 3; i++) { + push_call(obj); + shift_call(obj); +} + +%OptimizeFunctionOnNextCall(push_call); +%OptimizeFunctionOnNextCall(shift_call); +push_call(obj); +shift_call(obj); +assertOptimized(push_call); +assertOptimized(shift_call); +Object.seal(obj); +assertThrows(function() { push_call(obj); }, TypeError); +assertThrows(function() { shift_call(obj); }, TypeError); +assertOptimized(push_call); +// shift() doesn't have a custom call generator, so deopt will occur. +assertUnoptimized(shift_call); +assertDoesNotThrow(function() { push_call(objControl); }); +assertDoesNotThrow(function() { shift_call(objControl); }); + +// Verify special behavior of splice on sealed objects. +obj = [1,2,3]; +Object.seal(obj); +assertDoesNotThrow(function() { obj.splice(0,1,100); }); +assertEquals(100, obj[0]); +assertDoesNotThrow(function() { obj.splice(0,2,1,2); }); +assertDoesNotThrow(function() { obj.splice(1,2,1,2); }); +// Count of items to delete is clamped by length. +assertDoesNotThrow(function() { obj.splice(1,2000,1,2); }); +assertThrows(function() { obj.splice(0,0,1); }, TypeError); +assertThrows(function() { obj.splice(1,2000,1,2,3); }, TypeError); diff --git a/test/mjsunit/regress/regress-2711.js b/test/mjsunit/regress/regress-2711.js index a58e789..d5ac2ba 100644 --- a/test/mjsunit/regress/regress-2711.js +++ b/test/mjsunit/regress/regress-2711.js @@ -27,7 +27,7 @@ // Test that frozen arrays don't let their length change var a = Object.freeze([1]); -a.push(2); +assertThrows(function() { a.push(2); }, TypeError); assertEquals(1, a.length); -a.push(2); +assertThrows(function() { a.push(2); }, TypeError); assertEquals(1, a.length); diff --git a/test/mjsunit/regress/regress-299979.js b/test/mjsunit/regress/regress-299979.js new file mode 100644 index 0000000..0afbcb3 --- /dev/null +++ b/test/mjsunit/regress/regress-299979.js @@ -0,0 +1,34 @@ +// Copyright 2013 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. + + +(function(){ + "use strict"; + var list = Object.freeze([1, 2, 3]); + assertThrows(function() { list.unshift(4); }, TypeError); + assertThrows(function() { list.shift(); }, TypeError); +})(); -- 2.7.4