[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor

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 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);
 
   // Fail if it is not a heap object.
   GotoIf(TaggedIsSmi(tagged), &invalid);
 
@@ skipping 356 matching lines @@
       MachineType::Uint32(), backing_store, WordShl(index_word, 2),
       old_value_word32, new_value_word32)));
 
   // 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
 }
 
+#define BINOP_BUILTIN(op)                                       \
+  TF_BUILTIN(Atomics##op, SharedArrayBufferBuiltinsAssembler) { \
+    Node* array = Parameter(Descriptor::kArray);                \
+    Node* index = Parameter(Descriptor::kIndex);                \
+    Node* value = Parameter(Descriptor::kValue);                \
+    Node* context = Parameter(Descriptor::kContext);            \
+    AtomicBinopBuiltinCommon(array, index, value, context,      \
+                             &CodeAssembler::Atomic##op,        \
+                             Runtime::kAtomics##op);            \
+  }
+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) {
+  Node* instance_type;
+  Node* backing_store;
+  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+
+  Node* index_integer;
+  Node* index_word32 =
+      ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
+  Node* array_length_word32 = TruncateTaggedToWord32(
+      context, LoadObjectField(array, JSTypedArray::kLengthOffset));
+  ValidateAtomicIndex(index_word32, array_length_word32, context);
+
+  Node* value_integer = ToInteger(context, value);

[Jarin, 2017/04/07 09:55:48]

This makes me a bit uneasy, are we sure this cannot go wrong?

Historically, doing validation before a potential call to JS has been source of
serious security bugs (several 100K$ in Pwnium & Pwn2own just for typed array
bugs) because the checked preconditions could be invalidated and attackers would
get access to arbitrary memory. With SharedArrayBuffers, this might be
impossible because there is no neutering and all the buffers are external, but
it still feels dangerous.

At the very least, there should be a comment explaining why the validated
conditions are still valid after a call to arbitrary JS. 

It would be worth considering rechecking the conditions in debug mode.
This should be also accompanied by tests that do call to javascript from here
and manipulate the typed array so that clusterfuzz has a seed to check this
(i.e., for index and value, feed objects that do funky stuff in their valueOf
method).

[binji, 2017/04/08 20:23:12]

I'm fairly certain this is safe, as a typedarray cannot change its length, and
the backing SAB can never be neutered or shrink. In the future when we add SAB
resizing for wasm the SAB may grow, but the backing store pointer will never
change.
Agreed.
Good idea. I noticed that TruncateTaggedToWord32 might also call out to JS,
which would create a validation loop if we actually wanted to check the typed
array length. But the length is always the internal value, regardless of whether
the user defines a new length getter. In fact, the runtime equivalents already
do CHECK_LT(index, NumberToSize(sta->length())) where NumberToSize has a check
that the value is a Smi or HeapNumber, so we should be able to do the same here.

[aseemgarg, 2017/04/10 20:23:37]

Moved the JS calls above the validations and added tests that try to neuter the
SAB.

On 2017/04/08 20:23:12, binji wrote:

+
+#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);
+
+  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,
+  };
+  Switch(instance_type, &other, case_values, case_labels,
+         arraysize(case_labels));
+
+  Bind(&i8);
+  Return(SmiFromWord32((this->*function)(MachineType::Int8(), backing_store,
+                                         index_word, value_word32)));
+
+  Bind(&u8);
+  Return(SmiFromWord32((this->*function)(MachineType::Uint8(), backing_store,
+                                         index_word, value_word32)));
+
+  Bind(&i16);
+  Return(
+      SmiFromWord32((this->*function)(MachineType::Int16(), backing_store,
+                                      WordShl(index_word, 1), value_word32)));
+
+  Bind(&u16);
+  Return(
+      SmiFromWord32((this->*function)(MachineType::Uint16(), backing_store,
+                                      WordShl(index_word, 1), value_word32)));
+
+  Bind(&i32);
+  Return(ChangeInt32ToTagged(
+      (this->*function)(MachineType::Int32(), backing_store,
+                        WordShl(index_word, 2), value_word32)));
+
+  Bind(&u32);
+  Return(ChangeUint32ToTagged(
+      (this->*function)(MachineType::Uint32(), backing_store,
+                        WordShl(index_word, 2), value_word32)));
+
+  // 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