[SAB] Validate index before value conversion

src/builtins/builtins-sharedarraybuffer-gen.cc

 // Copyright 2017 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/builtins/builtins-utils-gen.h"
 #include "src/builtins/builtins.h"
 #include "src/code-stub-assembler.h"
 #include "src/objects.h"
 
 namespace v8 {
 namespace internal {
 
 using compiler::Node;
 
 class SharedArrayBufferBuiltinsAssembler : public CodeStubAssembler {
  public:
   explicit SharedArrayBufferBuiltinsAssembler(
       compiler::CodeAssemblerState* state)
       : CodeStubAssembler(state) {}
 
  protected:
   typedef Node* (CodeAssembler::*AssemblerFunction)(MachineType type,
                                                     Node* base, Node* offset,
                                                     Node* value);
   void ValidateSharedTypedArray(Node* tagged, Node* context,
                                 Node** out_instance_type,
                                 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);
 };
 
 void SharedArrayBufferBuiltinsAssembler::ValidateSharedTypedArray(
     Node* tagged, Node* context, Node** out_instance_type,
     Node** out_backing_store) {
   Label not_float_or_clamped(this), invalid(this);
 
@@ skipping 39 matching lines @@
   Node* byte_offset = ChangeUint32ToWord(TruncateTaggedToWord32(
       context, LoadObjectField(tagged, JSArrayBufferView::kByteOffsetOffset)));
   *out_backing_store =
       IntPtrAdd(BitcastTaggedToWord(backing_store), byte_offset);
 }
 
 // https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.ValidateAtomicAccess
 Node* SharedArrayBufferBuiltinsAssembler::ConvertTaggedAtomicIndexToWord32(
     Node* tagged, Node* context, Node** number_index) {
   VARIABLE(var_result, MachineRepresentation::kWord32);
+  Label done(this), range_error(this);
 
-  // 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);
+  // 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);
 
-  Label if_numberissmi(this), if_numberisnotsmi(this);
-  Branch(TaggedIsSmi(*number_index), &if_numberissmi, &if_numberisnotsmi);
-
-  BIND(&if_numberissmi);
+  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();
 
   BIND(&check_passed);
 }
 
 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),
       other(this);
   int32_t case_values[] = {
       FIXED_INT8_ARRAY_TYPE,   FIXED_UINT8_ARRAY_TYPE, FIXED_INT16_ARRAY_TYPE,
       FIXED_UINT16_ARRAY_TYPE, FIXED_INT32_ARRAY_TYPE, FIXED_UINT32_ARRAY_TYPE,
   };
   Label* case_labels[] = {
       &i8, &u8, &i16, &u16, &i32, &u32,
@@ skipping 29 matching lines @@
   BIND(&other);
   Unreachable();
 }
 
 TF_BUILTIN(AtomicsStore, SharedArrayBufferBuiltinsAssembler) {
   Node* array = Parameter(Descriptor::kArray);
   Node* index = Parameter(Descriptor::kIndex);
   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* 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);
 
+  Node* value_integer = ToInteger(context, value);
+  Node* value_word32 = TruncateTaggedToWord32(context, value_integer);
+
+#if DEBUG

[Jakob Kummerow, 2017/04/12 11:47:33]

This sounds like a use case for CSA_ASSERT. If the sanity check fails, you want
to crash hard, not just throw a harmless exception.

  CSA_ASSERT(
      this,
      Uint32LessThan(
          index_word32,
          TruncateTaggedToWord32(
              context, LoadObjectField(array, JSTypedArray::kLengthOffset))));

(or SmiToWord32 instead of TruncateTaggedToWord32 if the stored length is
guaranteed to be a Smi).

[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[] = {
       FIXED_INT8_ARRAY_TYPE,   FIXED_UINT8_ARRAY_TYPE, FIXED_INT16_ARRAY_TYPE,
       FIXED_UINT16_ARRAY_TYPE, FIXED_INT32_ARRAY_TYPE, FIXED_UINT32_ARRAY_TYPE,
   };
   Label* case_labels[] = {
       &u8, &u8, &u16, &u16, &u32, &u32,
   };
   Switch(instance_type, &other, case_values, case_labels,
          arraysize(case_labels));
@@ skipping 17 matching lines @@
   BIND(&other);
   Unreachable();
 }
 
 TF_BUILTIN(AtomicsExchange, SharedArrayBufferBuiltinsAssembler) {
   Node* array = Parameter(Descriptor::kArray);
   Node* index = Parameter(Descriptor::kIndex);
   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* 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* value_integer = ToInteger(context, value);
+
+#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,
                      value_integer));
 #else
   Node* index_word = ChangeUint32ToWord(index_word32);
 
   Node* value_word32 = TruncateTaggedToWord32(context, value_integer);
 
   Label i8(this), u8(this), i16(this), u16(this), i32(this), u32(this),
@@ skipping 40 matching lines @@
 #endif  // V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64
 }
 
 TF_BUILTIN(AtomicsCompareExchange, SharedArrayBufferBuiltinsAssembler) {
   Node* array = Parameter(Descriptor::kArray);
   Node* index = Parameter(Descriptor::kIndex);
   Node* old_value = Parameter(Descriptor::kOldValue);
   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* 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* old_value_integer = ToInteger(context, old_value);
+  Node* new_value_integer = ToInteger(context, new_value);
+
+#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 a version of validation that
CSA_ASSERTs (instead of throwing).

[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
   Return(CallRuntime(Runtime::kAtomicsCompareExchange, context, array,
                      index_integer, old_value_integer, new_value_integer));
 #else
   Node* index_word = ChangeUint32ToWord(index_word32);
 
   Node* old_value_word32 = TruncateTaggedToWord32(context, old_value_integer);
 
@@ skipping 61 matching lines @@
 BINOP_BUILTIN(Add)
 BINOP_BUILTIN(Sub)
 BINOP_BUILTIN(And)
 BINOP_BUILTIN(Or)
 BINOP_BUILTIN(Xor)
 #undef BINOP_BUILTIN
 
 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
   Return(CallRuntime(runtime_function, context, array, index_integer,
                      value_integer));
 #else
   Node* index_word = ChangeUint32ToWord(index_word32);
 
   Node* value_word32 = TruncateTaggedToWord32(context, value_integer);
 
@@ skipping 39 matching lines @@
 
   // This shouldn't happen, we've already validated the type.
   Bind(&other);
   Unreachable();
 #endif  // V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_PPC64
         // || V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X
 }
 
 }  // namespace internal
 }  // namespace v8