src/builtins/builtins-sharedarraybuffer-gen.cc - Issue 2814753003: [SAB] Validate index before value conversion

Unified Diff: src/builtins/builtins-sharedarraybuffer-gen.cc

Issue 2814753003: [SAB] Validate index before value conversion (Closed)

Patch Set: use byte temp register only for int8/uint8 Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Index: src/builtins/builtins-sharedarraybuffer-gen.cc

diff --git a/src/builtins/builtins-sharedarraybuffer-gen.cc b/src/builtins/builtins-sharedarraybuffer-gen.cc

index 056dfc6e1a5c27c0f6c289b3af17ca57b2141c79..80ee1363cbe2e92c7d90e2167668dce188268ac8 100644

--- a/src/builtins/builtins-sharedarraybuffer-gen.cc

+++ b/src/builtins/builtins-sharedarraybuffer-gen.cc

@@ -27,8 +27,7 @@ class SharedArrayBufferBuiltinsAssembler : public CodeStubAssembler {

Node** out_backing_store);

Node* ConvertTaggedAtomicIndexToWord32(Node* tagged, Node* context,

Node** number_index);

- void ValidateAtomicIndex(Node* index_word, Node* array_length_word,

- Node* context);

+ void ValidateAtomicIndex(Node* array, Node* index_word, Node* context);

void AtomicBinopBuiltinCommon(Node* array, Node* index, Node* value,

Node* context, AssemblerFunction function,

Runtime::FunctionId runtime_function);

@@ -88,56 +87,35 @@ void SharedArrayBufferBuiltinsAssembler::ValidateSharedTypedArray(

Node* SharedArrayBufferBuiltinsAssembler::ConvertTaggedAtomicIndexToWord32(

Node* tagged, Node* context, Node** number_index) {

VARIABLE(var_result, MachineRepresentation::kWord32);

- // TODO(jkummerow): Skip ToNumber call when |tagged| is a number already.

- // Maybe this can be unified with other tagged-to-index conversions?

- // Why does this return an int32, and not an intptr?

- // Why is there the additional |number_index| output parameter?

- Callable to_number = CodeFactory::ToNumber(isolate());

- *number_index = CallStub(to_number, context, tagged);

- Label done(this, &var_result);

- Label if_numberissmi(this), if_numberisnotsmi(this);

- Branch(TaggedIsSmi(*number_index), &if_numberissmi, &if_numberisnotsmi);

- BIND(&if_numberissmi);

- {

- var_result.Bind(SmiToWord32(*number_index));

- Goto(&done);

- }

- BIND(&if_numberisnotsmi);

+ Label done(this), range_error(this);

+ // Returns word32 since index cannot be longer than a TypedArray length,

+ // which has a uint32 maximum.

+ // The |number_index| output parameter is used only for architectures that

+ // don't currently have a TF implementation and forward to runtime functions

+ // instead; they expect the value has already been coerced to an integer.

+ *number_index = ToSmiIndex(tagged, context, &range_error);

+ var_result.Bind(SmiToWord32(*number_index));

+ Goto(&done);

+ BIND(&range_error);

{

- Node* number_index_value = LoadHeapNumberValue(*number_index);

- Node* access_index = TruncateFloat64ToWord32(number_index_value);

- Node* test_index = ChangeInt32ToFloat64(access_index);

- Label if_indexesareequal(this), if_indexesarenotequal(this);

- Branch(Float64Equal(number_index_value, test_index), &if_indexesareequal,

- &if_indexesarenotequal);

- BIND(&if_indexesareequal);

- {

- var_result.Bind(access_index);

- Goto(&done);

- }

- BIND(&if_indexesarenotequal);

- {

- CallRuntime(Runtime::kThrowInvalidAtomicAccessIndexError, context);

- Unreachable();

- }

+ CallRuntime(Runtime::kThrowInvalidAtomicAccessIndexError, context);

+ Unreachable();

}

BIND(&done);

return var_result.value();

}

-void SharedArrayBufferBuiltinsAssembler::ValidateAtomicIndex(

- Node* index_word, Node* array_length_word, Node* context) {

+void SharedArrayBufferBuiltinsAssembler::ValidateAtomicIndex(Node* array,

+ Node* index_word,

+ Node* context) {

// Check if the index is in bounds. If not, throw RangeError.

Label check_passed(this);

- GotoIf(Uint32LessThan(index_word, array_length_word), &check_passed);

+ Node* array_length_word32 = TruncateTaggedToWord32(

+ context, LoadObjectField(array, JSTypedArray::kLengthOffset));

+ GotoIf(Uint32LessThan(index_word, array_length_word32), &check_passed);

CallRuntime(Runtime::kThrowInvalidAtomicAccessIndexError, context);

Unreachable();

@@ -150,17 +128,14 @@ TF_BUILTIN(AtomicsLoad, SharedArrayBufferBuiltinsAssembler) {

Node* index = Parameter(Descriptor::kIndex);

Node* context = Parameter(Descriptor::kContext);

- Node* index_integer;

- Node* index_word32 =

- ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);

Node* instance_type;

Node* backing_store;

ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

- Node* array_length_word32 = TruncateTaggedToWord32(

- context, LoadObjectField(array, JSTypedArray::kLengthOffset));

- ValidateAtomicIndex(index_word32, array_length_word32, context);

+ Node* index_integer;

+ Node* index_word32 =

+ ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);

+ ValidateAtomicIndex(array, index_word32, context);

Node* index_word = ChangeUint32ToWord(index_word32);

Label i8(this), u8(this), i16(this), u16(this), i32(this), u32(this),

@@ -210,25 +185,26 @@ TF_BUILTIN(AtomicsStore, SharedArrayBufferBuiltinsAssembler) {

Node* value = Parameter(Descriptor::kValue);

Node* context = Parameter(Descriptor::kContext);

- // The value_integer needs to be computed before the validations as the

- // ToInteger function can be potentially modified in JS to invalidate the

- // conditions. This is just a no-cost safety measure as SABs can't be neutered

- // or shrunk.

- Node* value_integer = ToInteger(context, value);

- Node* value_word32 = TruncateTaggedToWord32(context, value_integer);

+ Node* instance_type;

+ Node* backing_store;

+ ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

Node* index_integer;

Node* index_word32 =

ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);

+ ValidateAtomicIndex(array, index_word32, context);

+ Node* index_word = ChangeUint32ToWord(index_word32);

- Node* instance_type;

- Node* backing_store;

- ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

+ Node* value_integer = ToInteger(context, value);

+ Node* value_word32 = TruncateTaggedToWord32(context, value_integer);

- Node* array_length_word32 = TruncateTaggedToWord32(

- context, LoadObjectField(array, JSTypedArray::kLengthOffset));

- ValidateAtomicIndex(index_word32, array_length_word32, context);

- Node* index_word = ChangeUint32ToWord(index_word32);

+#if DEBUG

Jakob Kummerow 2017/04/12 11:47:33 This sounds like a use case for CSA_ASSERT. If the

binji 2017/04/12 18:43:44 Done, thanks! Didn't know about CSA_ASSERT.

+ // In Debug mode, we re-validate the index as a sanity check because

+ // ToInteger above calls out to JavaScript. A SharedArrayBuffer can't be

+ // neutered and the TypedArray length can't change either, so skipping this

+ // check in Release mode is safe.

+ ValidateAtomicIndex(array, index_word32, context);

+#endif

Label u8(this), u16(this), u32(this), other(this);

int32_t case_values[] = {

@@ -267,23 +243,24 @@ TF_BUILTIN(AtomicsExchange, SharedArrayBufferBuiltinsAssembler) {

Node* value = Parameter(Descriptor::kValue);

Node* context = Parameter(Descriptor::kContext);

- // The value_integer needs to be computed before the validations as the

- // ToInteger function can be potentially modified in JS to invalidate the

- // conditions. This is just a no-cost safety measure as SABs can't be neutered

- // or shrunk.

- Node* value_integer = ToInteger(context, value);

+ Node* instance_type;

+ Node* backing_store;

+ ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

Node* index_integer;

Node* index_word32 =

ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);

+ ValidateAtomicIndex(array, index_word32, context);

- Node* instance_type;

- Node* backing_store;

- ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

+ Node* value_integer = ToInteger(context, value);

- Node* array_length_word32 = TruncateTaggedToWord32(

- context, LoadObjectField(array, JSTypedArray::kLengthOffset));

- ValidateAtomicIndex(index_word32, array_length_word32, context);

+#if DEBUG

+ // In Debug mode, we re-validate the index as a sanity check because

+ // ToInteger above calls out to JavaScript. A SharedArrayBuffer can't be

+ // neutered and the TypedArray length can't change either, so skipping this

+ // check in Release mode is safe.

+ ValidateAtomicIndex(array, index_word32, context);

+#endif

#if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64

Return(CallRuntime(Runtime::kAtomicsExchange, context, array, index_integer,

@@ -344,24 +321,25 @@ TF_BUILTIN(AtomicsCompareExchange, SharedArrayBufferBuiltinsAssembler) {

Node* new_value = Parameter(Descriptor::kNewValue);

Node* context = Parameter(Descriptor::kContext);

- // The value_integers needs to be computed before the validations as the

- // ToInteger function can be potentially modified in JS to invalidate the

- // conditions. This is just a no-cost safety measure as SABs can't be neutered

- // or shrunk.

- Node* old_value_integer = ToInteger(context, old_value);

- Node* new_value_integer = ToInteger(context, new_value);

+ Node* instance_type;

+ Node* backing_store;

+ ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

Node* index_integer;

Node* index_word32 =

ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);

+ ValidateAtomicIndex(array, index_word32, context);

- Node* instance_type;

- Node* backing_store;

- ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

+ Node* old_value_integer = ToInteger(context, old_value);

+ Node* new_value_integer = ToInteger(context, new_value);

- Node* array_length_word32 = TruncateTaggedToWord32(

- context, LoadObjectField(array, JSTypedArray::kLengthOffset));

- ValidateAtomicIndex(index_word32, array_length_word32, context);

+#if DEBUG

+ // In Debug mode, we re-validate the index as a sanity check because

+ // ToInteger above calls out to JavaScript. A SharedArrayBuffer can't be

+ // neutered and the TypedArray length can't change either, so skipping this

+ // check in Release mode is safe.

+ ValidateAtomicIndex(array, index_word32, context);

Jarin 2017/04/12 04:39:01 Nit: It would be even better if we could use here

binji 2017/04/12 18:43:44 Done.

+#endif

#if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_PPC64 || \

V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X

@@ -443,23 +421,24 @@ BINOP_BUILTIN(Xor)

void SharedArrayBufferBuiltinsAssembler::AtomicBinopBuiltinCommon(

Node* array, Node* index, Node* value, Node* context,

AssemblerFunction function, Runtime::FunctionId runtime_function) {

- // The value_integer needs to be computed before the validations as the

- // ToInteger function can be potentially modified in JS to invalidate the

- // conditions. This is just a no-cost safety measure as SABs can't be neutered

- // or shrunk.

- Node* value_integer = ToInteger(context, value);

+ Node* instance_type;

+ Node* backing_store;

+ ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

Node* index_integer;

Node* index_word32 =

ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);

+ ValidateAtomicIndex(array, index_word32, context);

- Node* instance_type;

- Node* backing_store;

- ValidateSharedTypedArray(array, context, &instance_type, &backing_store);

+ Node* value_integer = ToInteger(context, value);

- Node* array_length_word32 = TruncateTaggedToWord32(

- context, LoadObjectField(array, JSTypedArray::kLengthOffset));

- ValidateAtomicIndex(index_word32, array_length_word32, context);

+#if DEBUG

+ // In Debug mode, we re-validate the index as a sanity check because

+ // ToInteger above calls out to JavaScript. A SharedArrayBuffer can't be

+ // neutered and the TypedArray length can't change either, so skipping this

+ // check in Release mode is safe.

+ ValidateAtomicIndex(array, index_word32, context);

+#endif

#if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_PPC64 || \

V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X

« no previous file with comments | « src/builtins/builtins-sharedarraybuffer.cc ('k') | src/compiler/ia32/code-generator-ia32.cc » ('j') | test/mjsunit/harmony/atomics.js » ('J')