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

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 04c2133c636c139b63ec3366b3c3922e85078252..79722f5b845c232fbe0a682586046d7caa668cce 100644
--- a/src/builtins/builtins-sharedarraybuffer-gen.cc
+++ b/src/builtins/builtins-sharedarraybuffer-gen.cc
@@ -19,6 +19,9 @@ class SharedArrayBufferBuiltinsAssembler : public CodeStubAssembler {
       : 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);
@@ -26,6 +29,9 @@ class SharedArrayBufferBuiltinsAssembler : public CodeStubAssembler {
                                          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(
@@ -402,5 +408,94 @@ TF_BUILTIN(AtomicsCompareExchange, SharedArrayBufferBuiltinsAssembler) {
         // || 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