Fix range checks for typed data in native code and the optimizer.

Fix range checks for typed data in native code and the optimizer.

TEST=tests/standalone/typed_data_test.dart
R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27801

[Ivan Posva, 2013-09-20 17:20:30]

https://codereview.chromium.org/24239003/diff/5001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/24239003/diff/5001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2652: // Check byte_index + num_bytes <
len_in_bytes.
Where is the check that byte_index is positive?

[siva, 2013-09-20 22:03:45]

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
File runtime/lib/typed_data.cc (right):

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
runtime/lib/typed_data.cc:22: intptr_t element_size_in_bytes) {
indentation

For a little more readability I am wondering if we should rename
'num_bytes' to 'access_length_in_bytes'

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
runtime/lib/typed_data.cc:26: (offset_in_bytes / element_size_in_bytes) +
num_bytes,
I think this should be
((offset_in_bytes + num_bytes) / element_size_in_bytes)

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
runtime/lib/typed_data.cc:88: element_size_in_bytes);
indentation

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
runtime/lib/typed_data.cc:92: element_size_in_bytes);
indentation

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
runtime/lib/typed_data.cc:205: GET_NON_NULL_NATIVE_ARGUMENT(object, value,
arguments->NativeArgAt(2));      \
maybe more readable to use elem_size_in_bytes instead of num_bytes

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/lib/typed_d...
runtime/lib/typed_data.cc:208: RangeCheck(offsetInBytes.Value(), num_bytes,
array.LengthInBytes(), 1);    \
shouldn't this be num_bytes instead of '1' here?

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/24239003/diff/5001/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:2652: // Check byte_index + num_bytes <
len_in_bytes.
We have been using the name 'element_size_in_bytes' for this consistently, could
we call this 'element_size_in_bytes' instead of 'num_bytes'

[Florian Schneider, 2013-09-23 11:02:26]

Addressed comments and added tests with small negative index and for Int64 and
Uint64List.

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc
File runtime/lib/typed_data.cc (right):

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:22: intptr_t element_size_in_bytes) {
On 2013/09/20 22:03:45, siva wrote:
> indentation
> 
> For a little more readability I am wondering if we should rename
> 'num_bytes' to 'access_length_in_bytes'

Done.

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:26: (offset_in_bytes / element_size_in_bytes) +
num_bytes,
On 2013/09/20 22:03:45, siva wrote:
> I think this should be
> ((offset_in_bytes + num_bytes) / element_size_in_bytes)

Done.

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:88: element_size_in_bytes);
On 2013/09/20 22:03:45, siva wrote:
> indentation

Done.

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:92: element_size_in_bytes);
On 2013/09/20 22:03:45, siva wrote:
> indentation

Done.

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:205: GET_NON_NULL_NATIVE_ARGUMENT(object, value,
arguments->NativeArgAt(2));      \
On 2013/09/20 22:03:45, siva wrote:
> maybe more readable to use elem_size_in_bytes instead of num_bytes

Changed to access_size.

https://codereview.chromium.org/24239003/diff/5001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:208: RangeCheck(offsetInBytes.Value(), num_bytes,
array.LengthInBytes(), 1);    \
On 2013/09/20 22:03:45, siva wrote:
> shouldn't this be num_bytes instead of '1' here?

Done.

https://codereview.chromium.org/24239003/diff/5001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/24239003/diff/5001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2652: // Check byte_index + num_bytes <
len_in_bytes.
On 2013/09/20 22:03:45, siva wrote:
> We have been using the name 'element_size_in_bytes' for this consistently,
could
> we call this 'element_size_in_bytes' instead of 'num_bytes'

Changed to access_size.

https://codereview.chromium.org/24239003/diff/5001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2652: // Check byte_index + num_bytes <
len_in_bytes.
On 2013/09/20 17:20:30, Ivan Posva wrote:
> Where is the check that byte_index is positive?

Good catch. Changed into two checks using CheckArrayBoundInstr and added a test:

0 <= element_size - 1 < length

and

0 <= byte_index < length - (element_size - 1)

[siva, 2013-09-24 00:15:27]

lgtm

[Florian Schneider, 2013-09-24 08:22:02]

Committed patchset #7 manually as r27801 (presubmit successful).