Error checking for length parameter of external array constructors in shell

samples/shell.cc

Index: samples/shell.cc
diff --git a/samples/shell.cc b/samples/shell.cc
index 950370adaa80fda9bd7896a41a0c3f32e7e69eaa..72a00f7d7841d7e0ac5eb3dac46d0963a391778e 100644
--- a/samples/shell.cc
+++ b/samples/shell.cc
@@ -497,14 +497,31 @@ void ExternalArrayWeakCallback(v8::Persistent<v8::Value> object, void* data) {
 
 v8::Handle<v8::Value> CreateExternalArray(const v8::Arguments& args,
                                           v8::ExternalArrayType type,
-                                          int element_size) {
+                                          size_t element_size) {

[Lasse Reichstein, 2011/06/28 09:28:58]

Let's ASSERT that element_size it's a meaningful value (like, always a positive
power of two and no bigger than 8).

In which case you don't need to change the type in the interface (although I
personally support using size_t for sizes everywhere :).

[Jakob Kummerow, 2011/06/28 14:02:38]

Done.
Changed the interface to size_t anyway because it's more consistent.

   if (args.Length() != 1) {
     return v8::ThrowException(
         v8::String::New("Array constructor needs one parameter."));
   }
-  int length = args[0]->Int32Value();
-  void* data = malloc(length * element_size);
-  memset(data, 0, length * element_size);
+  if (args[0]->Int32Value() < 0) {

[Lasse Reichstein, 2011/06/28 09:28:58]

You convert args[0] to int32 twice. Just do it once and save the value ...
except that I don't think you want the int32 value in this check.
Instead, do a proper overflow check on the value (if someone passes in a double
number with value kMaxLength + 2 * kMaxInt, it should be rejected, not silently
converted to kMaxLength-2).

[Lasse Reichstein, 2011/06/28 09:44:12]

That is, ofcourse, unless there is a specification that says to convert args[0]
using ToInt32. In that case, just do it once.

[Jakob Kummerow, 2011/06/28 14:02:38]

Spec says the c'tor argument is an "unsigned long", so I think ToInt32 should be
fine, considering the v8-internal maximum value we check against anyway (see
below).

Converting only once: done.

[Lasse Reichstein, 2011/06/29 08:51:05]

An unsigned long can take values that are too big (e.g., 2^32), and which get
silently truncated by converting to ToInt32, so I think you should do the range
testing before converting.

+    return v8::ThrowException(
+        v8::String::New("Array length must not be negative."));
+  }
+  size_t length = static_cast<size_t>(args[0]->Int32Value());
+  if (length > static_cast<size_t>(v8::internal::ExternalArray::kMaxLength)) {

[Lasse Reichstein, 2011/06/28 09:28:58]

If both values are int32, there's no need to convert them both to size_t before
checking.

Is the kMaxLength independent of element size? I would have expected a kMaxSize
(max memory use) and a max length derived from that (by subtracting any overhead
and dividing by element size).

[Jakob Kummerow, 2011/06/28 14:02:38]

True. Removed the cast.
Nope, kMaxLength is type independent. 

+    return v8::ThrowException(
+        v8::String::New("Array length exceeds maximum length."));
+  }
+  size_t malloc_size = length * element_size;
+  // Check for overflow in the multiplication.
+  if (malloc_size / length != element_size) {
+    return v8::ThrowException(
+        v8::String::New("Array size exceeds memory limit."));

[Lasse Reichstein, 2011/06/28 09:28:58]

Can this happen?
I.e., is kMaxLength * maximal element size ever not representable as a size_t.
Otherwise, just make an ASSERT.

[Jakob Kummerow, 2011/06/28 14:02:38]

It can happen. kMaxLength is 2^30 - 1, and Float64Array's element_size is 8,
resulting in 8GB theoretical max size, which a 32bit VM obviously won't let us
allocate. 
However, by using calloc() below, this manual check becomes superfluous.

+  }
+  void* data = malloc(malloc_size);

[Lasse Reichstein, 2011/06/28 09:28:58]

How about using calloc instead? It seems like just the thing for allocating
arrays, and it even initializes the memory to zero.

[Jakob Kummerow, 2011/06/28 14:02:38]

Done, thanks for the hint.

+  if (data == NULL) {
+    return v8::ThrowException(v8::String::New("Memory allocation failed."));
+  }
+  memset(data, 0, malloc_size);
   v8::Handle<v8::Object> array = v8::Object::New();
   v8::Persistent<v8::Object> persistent_array =
       v8::Persistent<v8::Object>::New(array);