Error checking for length parameter of external array constructors in shell

Error checking for length parameter of external array constructors in shell

BUG=v8:1501
Committed: http://code.google.com/p/v8/source/detail?r=8458

[Jakob Kummerow, 2011-06-28 09:06:58]

PTAL.

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

LGTM with comments.

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode500
samples/shell.cc:500: size_t element_size) {
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 :).

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode505
samples/shell.cc:505: if (args[0]->Int32Value() < 0) {
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).

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode510
samples/shell.cc:510: if (length >
static_cast<size_t>(v8::internal::ExternalArray::kMaxLength)) {
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).

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode518
samples/shell.cc:518: v8::String::New("Array size exceeds memory limit."));
Can this happen?
I.e., is kMaxLength * maximal element size ever not representable as a size_t.
Otherwise, just make an ASSERT.

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode520
samples/shell.cc:520: void* data = malloc(malloc_size);
How about using calloc instead? It seems like just the thing for allocating
arrays, and it even initializes the memory to zero.

[Lasse Reichstein, 2011-06-28 09:44:08]

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode505
samples/shell.cc:505: if (args[0]->Int32Value() < 0) {
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]

Thanks for the review. I've addressed your comments. 

If there's anything else, I'm happy to discuss this in person to reduce latency
and increase interactiveness :-)

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode500
samples/shell.cc:500: size_t element_size) {
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
> 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 :).

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

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode505
samples/shell.cc:505: if (args[0]->Int32Value() < 0) {
On 2011/06/28 09:44:12, Lasse Reichstein wrote:
> That is, ofcourse, unless there is a specification that says to convert
args[0]
> using ToInt32. In that case, just do it once.

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.

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode510
samples/shell.cc:510: if (length >
static_cast<size_t>(v8::internal::ExternalArray::kMaxLength)) {
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
> If both values are int32, there's no need to convert them both to size_t
before
> checking.

True. Removed the cast.

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

Nope, kMaxLength is type independent.

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode518
samples/shell.cc:518: v8::String::New("Array size exceeds memory limit."));
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
> Can this happen?
> I.e., is kMaxLength * maximal element size ever not representable as a size_t.
> Otherwise, just make an ASSERT.

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.

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode520
samples/shell.cc:520: void* data = malloc(malloc_size);
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
> How about using calloc instead? It seems like just the thing for allocating
> arrays, and it even initializes the memory to zero.

Done, thanks for the hint.

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

LGTM

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode505
samples/shell.cc:505: if (args[0]->Int32Value() < 0) {
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.

[Jakob Kummerow, 2011-06-29 10:28:04]

landed.