[SAB] Validate index before value conversion

src/builtins/builtins-sharedarraybuffer-gen.cc

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