Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(342)

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

Issue 2814753003: [SAB] Validate index before value conversion (Closed)
Patch Set: feedback 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
Download patch
« no previous file with comments | « src/builtins/builtins-sharedarraybuffer.cc ('k') | src/compiler/ia32/code-generator-ia32.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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..a9331a15484c417838c7113f994a07186ba9e7b2 100644
--- a/src/builtins/builtins-sharedarraybuffer-gen.cc
+++ b/src/builtins/builtins-sharedarraybuffer-gen.cc
@@ -27,8 +27,11 @@ 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);
+#if DEBUG
+ void DebugSanityCheckAtomicIndex(Node* array, Node* index_word,
+ Node* context);
+#endif
void AtomicBinopBuiltinCommon(Node* array, Node* index, Node* value,
Node* context, AssemblerFunction function,
Runtime::FunctionId runtime_function);
@@ -88,56 +91,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);
+ 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);
{
- var_result.Bind(SmiToWord32(*number_index));
- Goto(&done);
- }
-
- BIND(&if_numberisnotsmi);
- {
- 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();
@@ -145,22 +127,35 @@ void SharedArrayBufferBuiltinsAssembler::ValidateAtomicIndex(
BIND(&check_passed);
}
+#if DEBUG
+void SharedArrayBufferBuiltinsAssembler::DebugSanityCheckAtomicIndex(
+ Node* array, Node* index_word, Node* context) {
+ // 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.
+ CSA_ASSERT(
+ this,
+ Uint32LessThan(
+ index_word,
+ TruncateTaggedToWord32(
+ context, LoadObjectField(array, JSTypedArray::kLengthOffset))));
+}
+#endif
+
TF_BUILTIN(AtomicsLoad, SharedArrayBufferBuiltinsAssembler) {
Node* array = Parameter(Descriptor::kArray);
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 +205,22 @@ 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
+ DebugSanityCheckAtomicIndex(array, index_word32, context);
+#endif
Label u8(this), u16(this), u32(this), other(this);
int32_t case_values[] = {
@@ -267,23 +259,20 @@ 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
+ DebugSanityCheckAtomicIndex(array, index_word32, context);
+#endif
#if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64
Return(CallRuntime(Runtime::kAtomicsExchange, context, array, index_integer,
@@ -344,24 +333,21 @@ 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
+ DebugSanityCheckAtomicIndex(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
@@ -443,23 +429,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') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698