Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

TypedArrays: Fix size/index overflow on 32-bit. #4550

Closed
wants to merge 5 commits into from

4 participants

Dean McNamee Ben Noordhuis Nodejs Jenkins Timothy J Fontaine
Dean McNamee

Depends on #4547

deanm added some commits
Dean McNamee deanm Remove TypedArrays get().
This seems to have been added as a result of misreading the spec, there is no
get() method, only a getter (which the spec names get()), but this is actually
the [] operator.  There are many webkit tests that explicitly test for the
fact that the get() method is abscent.  Remove it to conform to the spec.
18ea925
Dean McNamee deanm Remove TypedArrays set(index, val) and match WebKit exception strings.
It seems that like get(), set(index, val) was added as a misreading of the spec.
There are only two set() methods defined in the spec:
    void set(TypedArray array, optional unsigned long offset)
    void set(type[] array, optional unsigned long offset)
The set(index, val) is handled by the []= operator.
Additionally updated a few exception error strings to match WebKit.
254907b
Dean McNamee deanm TypedArrays: Update a few c casts to C++ static_casts. e703b0c
Dean McNamee deanm TypedArrays: Update some exception messages to match WebKit. a203899
Dean McNamee deanm TypedArrays: Fix size/index overflow on 32-bit.
On 64-bit, the calculation is promoted to a 64-bit int.  However on 32-bit the
int will be 32-bit and the index + size calculation can overflow.  Rearrange
the math to prevent overflow and add a few more checks for additional safety.
The size should never be less than 0, for example, but we check anyway.
a6781cc
Ben Noordhuis

For posterity, this PR doesn't apply to v0.8 so I've landed a fix for the overflow bug in ed825f4.

Dean McNamee

Yeah, it depends on #4547. Perhaps I will make a future patch that cleans up overall some of the signed/unsigned-ness throughout the code. A bit awkward sometimes between the V8 api and the TypedArray spec.

Thanks for patching it against 0.8.

Nodejs Jenkins

Can one of the admins verify this patch?

Timothy J Fontaine
Owner

We now use typed arrays directly from V8, thanks!

Timothy J Fontaine tjfontaine closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 9, 2013
  1. Dean McNamee

    Remove TypedArrays get().

    deanm authored
    This seems to have been added as a result of misreading the spec, there is no
    get() method, only a getter (which the spec names get()), but this is actually
    the [] operator.  There are many webkit tests that explicitly test for the
    fact that the get() method is abscent.  Remove it to conform to the spec.
  2. Dean McNamee

    Remove TypedArrays set(index, val) and match WebKit exception strings.

    deanm authored
    It seems that like get(), set(index, val) was added as a misreading of the spec.
    There are only two set() methods defined in the spec:
        void set(TypedArray array, optional unsigned long offset)
        void set(type[] array, optional unsigned long offset)
    The set(index, val) is handled by the []= operator.
    Additionally updated a few exception error strings to match WebKit.
  3. Dean McNamee
  4. Dean McNamee
  5. Dean McNamee

    TypedArrays: Fix size/index overflow on 32-bit.

    deanm authored
    On 64-bit, the calculation is promoted to a 64-bit int.  However on 32-bit the
    int will be 32-bit and the index + size calculation can overflow.  Rearrange
    the math to prevent overflow and add a few more checks for additional safety.
    The size should never be less than 0, for example, but we check anyway.
This page is out of date. Refresh to see the latest.
Showing with 80 additions and 91 deletions.
  1. +73 −84 src/v8_typed_array.cc
  2. +7 −7 test/simple/test-typed-arrays.js
157 src/v8_typed_array.cc
View
@@ -131,7 +131,7 @@ class ArrayBuffer {
static v8::Handle<v8::Value> slice(const v8::Arguments& args) {
if (args.Length() < 1)
- return ThrowError("Wrong number of arguments.");
+ return ThrowTypeError("Not enough arguments");
unsigned int length =
args.This()->Get(v8::String::New("byteLength"))->Uint32Value();
@@ -217,7 +217,6 @@ class TypedArray {
v8::Local<v8::Signature> default_signature = v8::Signature::New(ft_cache);
static BatchedMethods methods[] = {
- { "get", &TypedArray<TBytes, TEAType>::get },
{ "set", &TypedArray<TBytes, TEAType>::set },
{ "slice", &TypedArray<TBytes, TEAType>::subarray },
{ "subarray", &TypedArray<TBytes, TEAType>::subarray },
@@ -350,77 +349,63 @@ class TypedArray {
return args.This();
}
- static v8::Handle<v8::Value> get(const v8::Arguments& args) {
- if (args.Length() < 1)
- return ThrowError("Wrong number of arguments.");
-
- if (args[0]->IsNumber())
- return args.This()->Get(args[0]->Uint32Value());
-
- return v8::Undefined();
- }
-
static v8::Handle<v8::Value> set(const v8::Arguments& args) {
if (args.Length() < 1)
- return ThrowError("Wrong number of arguments.");
-
- //if (!args[0]->IsObject())
- // return ThrowTypeError("Type error.");
-
- if (args[0]->IsNumber()) { // index, <type> value
- args.This()->Set(args[0]->Uint32Value(), args[1]);
- } else if (args[0]->IsObject()) {
- v8::Handle<v8::Object> obj = v8::Handle<v8::Object>::Cast(args[0]);
-
- if (TypedArray<TBytes, TEAType>::HasInstance(obj)) { // ArrayBufferView.
- if (args[1]->Int32Value() < 0)
- return ThrowRangeError("Offset may not be negative.");
-
- unsigned int offset = args[1]->Uint32Value();
- unsigned int src_length =
- obj->Get(v8::String::New("length"))->Uint32Value();
- unsigned int dst_length =
- args.This()->Get(v8::String::New("length"))->Uint32Value();
- if (offset > dst_length)
- return ThrowRangeError("Offset out of range.");
-
- if (src_length > dst_length - offset)
- return ThrowRangeError("Offset/length out of range.");
-
- // We don't want to get the buffer pointer, because that means we'll have
- // to just do the calculations for byteOffset / byteLength again.
- // Instead just use the pointer on the external array data.
- void* src_ptr = obj->GetIndexedPropertiesExternalArrayData();
- void* dst_ptr = args.This()->GetIndexedPropertiesExternalArrayData();
-
- // From the spec:
- // If the input array is a TypedArray, the two arrays may use the same
- // underlying ArrayBuffer. In this situation, setting the values takes
- // place as if all the data is first copied into a temporary buffer that
- // does not overlap either of the arrays, and then the data from the
- // temporary buffer is copied into the current array.
- memmove(reinterpret_cast<char*>(dst_ptr) + offset * TBytes, src_ptr,
- src_length * TBytes);
- } else { // type[]
- if (args[1]->Int32Value() < 0)
- return ThrowRangeError("Offset may not be negative.");
-
- unsigned int src_length =
- obj->Get(v8::String::New("length"))->Uint32Value();
- unsigned int dst_length =
- args.This()->Get(v8::String::New("length"))->Uint32Value();
- unsigned int offset = args[1]->Uint32Value();
-
- if (offset > dst_length)
- return ThrowRangeError("Offset out of range.");
-
- if (src_length > dst_length - offset)
- return ThrowRangeError("Offset/length out of range.");
-
- for (uint32_t i = 0; i < src_length; ++i) {
- // Use the v8 setter to deal with typing. Maybe slow?
- args.This()->Set(i + offset, obj->Get(i));
- }
+ return ThrowTypeError("Not enough arguments");
+
+ if (!args[0]->IsObject())
+ return ThrowTypeError("Invalid argument");
+
+ v8::Handle<v8::Object> obj = v8::Handle<v8::Object>::Cast(args[0]);
+
+ if (TypedArray<TBytes, TEAType>::HasInstance(obj)) { // ArrayBufferView.
+ if (args[1]->Int32Value() < 0)
+ return ThrowRangeError("Offset may not be negative.");
+
+ unsigned int offset = args[1]->Uint32Value();
+ unsigned int src_length =
+ obj->Get(v8::String::New("length"))->Uint32Value();
+ unsigned int dst_length =
+ args.This()->Get(v8::String::New("length"))->Uint32Value();
+ if (offset > dst_length)
+ return ThrowRangeError("Offset out of range.");
+
+ if (src_length > dst_length - offset)
+ return ThrowRangeError("Index is out of range.");
+
+ // We don't want to get the buffer pointer, because that means we'll have
+ // to just do the calculations for byteOffset / byteLength again.
+ // Instead just use the pointer on the external array data.
+ void* src_ptr = obj->GetIndexedPropertiesExternalArrayData();
+ void* dst_ptr = args.This()->GetIndexedPropertiesExternalArrayData();
+
+ // From the spec:
+ // If the input array is a TypedArray, the two arrays may use the same
+ // underlying ArrayBuffer. In this situation, setting the values takes
+ // place as if all the data is first copied into a temporary buffer that
+ // does not overlap either of the arrays, and then the data from the
+ // temporary buffer is copied into the current array.
+ memmove(reinterpret_cast<char*>(dst_ptr) + offset * TBytes, src_ptr,
+ src_length * TBytes);
+ } else { // type[]
+ if (args[1]->Int32Value() < 0)
+ return ThrowRangeError("Offset may not be negative.");
+
+ unsigned int src_length =
+ obj->Get(v8::String::New("length"))->Uint32Value();
+ unsigned int dst_length =
+ args.This()->Get(v8::String::New("length"))->Uint32Value();
+ unsigned int offset = args[1]->Uint32Value();
+
+ if (offset > dst_length)
+ return ThrowRangeError("Offset out of range.");
+
+ if (src_length > dst_length - offset)
+ return ThrowRangeError("Index is out of range.");
+
+ for (uint32_t i = 0; i < src_length; ++i) {
+ // Use the v8 setter to deal with typing. Maybe slow?
+ args.This()->Set(i + offset, obj->Get(i));
}
}
@@ -439,11 +424,11 @@ class TypedArray {
if (begin < 0) begin = length + begin;
if (begin < 0) begin = 0;
- if ((unsigned)begin > length) begin = length;
+ if (static_cast<unsigned int>(begin) > length) begin = length;
if (end < 0) end = length + end;
if (end < 0) end = 0;
- if ((unsigned)end > length) end = length;
+ if (static_cast<unsigned int>(end) > length) end = length;
if (begin > end) begin = end;
@@ -617,7 +602,7 @@ class DataView {
return ThrowTypeError("Constructor cannot be called as a function.");
if (args.Length() < 1)
- return ThrowError("Wrong number of arguments.");
+ return ThrowTypeError("Not enough arguments");
if (!args[0]->IsObject())
return ThrowError("Object must be an ArrayBuffer.");
@@ -631,16 +616,16 @@ class DataView {
unsigned int byte_offset = args[1]->Uint32Value();
if (args[1]->Int32Value() < 0 || byte_offset >= byte_length)
- return ThrowRangeError("byteOffset out of range.");
+ return ThrowRangeError("Size is too large (or is negative).");
if (!args[2]->IsUndefined()) {
if (args[2]->Int32Value() < 0)
- return ThrowRangeError("byteLength out of range.");
+ return ThrowRangeError("Size is too large (or is negative).");
unsigned int new_byte_length = args[2]->Uint32Value();
if (new_byte_length > byte_length)
- return ThrowRangeError("byteLength out of range.");
+ return ThrowRangeError("Size is too large (or is negative).");
if (byte_offset + new_byte_length > byte_length)
- return ThrowRangeError("byteOffset/byteLength out of range.");
+ return ThrowRangeError("Size is too large (or is negative).");
byte_length = new_byte_length;
} else {
// Adjust the original byte_length from total length to length to end.
@@ -669,7 +654,7 @@ class DataView {
template <typename T>
static v8::Handle<v8::Value> getGeneric(const v8::Arguments& args) {
if (args.Length() < 1)
- return ThrowError("Wrong number of arguments.");
+ return ThrowTypeError("Not enough arguments");
unsigned int index = args[0]->Uint32Value();
bool little_endian = args[1]->BooleanValue();
@@ -679,8 +664,10 @@ class DataView {
int size = args.This()->GetIndexedPropertiesExternalArrayDataLength() *
element_size;
- if (index + sizeof(T) > (unsigned)size) // TODO(deanm): integer overflow.
- return ThrowError("Index out of range.");
+ if (size <= 0 || static_cast<unsigned int>(size) < sizeof(T) ||
+ index > static_cast<unsigned int>(size) - sizeof(T)) {
+ return ThrowError("IndexSizeError: DOM Exception 1");
+ }
void* ptr = reinterpret_cast<char*>(
args.This()->GetIndexedPropertiesExternalArrayData()) + index;
@@ -701,7 +688,7 @@ class DataView {
template <typename T>
static v8::Handle<v8::Value> setGeneric(const v8::Arguments& args) {
if (args.Length() < 2)
- return ThrowError("Wrong number of arguments.");
+ return ThrowTypeError("Not enough arguments");
unsigned int index = args[0]->Int32Value();
bool little_endian = args[2]->BooleanValue();
@@ -711,8 +698,10 @@ class DataView {
int size = args.This()->GetIndexedPropertiesExternalArrayDataLength() *
element_size;
- if (index + sizeof(T) > (unsigned)size) // TODO(deanm): integer overflow.
- return ThrowError("Index out of range.");
+ if (size <= 0 || static_cast<unsigned int>(size) < sizeof(T) ||
+ index > static_cast<unsigned int>(size) - sizeof(T)) {
+ return ThrowError("IndexSizeError: DOM Exception 1");
+ }
void* ptr = reinterpret_cast<char*>(
args.This()->GetIndexedPropertiesExternalArrayData()) + index;
14 test/simple/test-typed-arrays.js
View
@@ -153,13 +153,13 @@ sub[1] = 0x34;
assert.equal(uint8[2], 0x12);
assert.equal(uint8[3], 0x34);
-// test .set(index, value), .set(arr, offset) and .get(index)
-uint8.set(1, 0x09);
+// test .set(index, value), .set(arr, offset)
+uint8[1] = 0x09;
uint8.set([0x0a, 0x0b], 2);
-assert.equal(uint8.get(1), 0x09);
-assert.equal(uint8.get(2), 0x0a);
-assert.equal(uint8.get(3), 0x0b);
+assert.equal(uint8[1], 0x09);
+assert.equal(uint8[2], 0x0a);
+assert.equal(uint8[3], 0x0b);
// test clamped array
var uint8c = new Uint8ClampedArray(buffer);
@@ -169,8 +169,8 @@ uint8c[1] = 257;
assert.equal(uint8c[0], 0);
assert.equal(uint8c[1], 255);
-uint8c.set(0, -10);
-uint8c.set(1, 260);
+uint8c[0] = -10;
+uint8c[1] = 260;
assert.equal(uint8c[0], 0);
assert.equal(uint8c[1], 255);
Something went wrong with that request. Please try again.