Bug 1131996 - Part 2: Support out-of-bounds read access on arguments objects in CacheIR. r=iain

The `ELEMENT_OVERRIDDEN_BIT` flag is set whenever any element is defined on an
arguments object, irrespective of whether the element is in-bounds or out-of-bounds.
That means that flag can also be used to determine if an arguments object has any
elements besides the frame arguments.

When reading a possible out-of-bounds index, we can therefore use the following
approach:
1. Fail whenever `ELEMENT_OVERRIDDEN_BIT` is set.
2. If the index is in-bounds:
  a. Return the in-bounds element unless it's `FORWARD_TO_CALL_SLOT`.
3. Else,
  a. Fail if the index is less than zero.
  b. Return `undefined`.

Plus a prototype guard check to ensure the element isn't present on any object
of the prototype chain.

Differential Revision: https://phabricator.services.mozilla.com/D129620

js/src/jit-test/tests/arguments/args-mutate-proto-1.js

@@ -0,0 +1,15 @@
+function outOfBounds() {
+  var proto = ["pass"];
+
+  var N = 100;
+  for (var i = 0; i <= N; ++i) {
+    if (i === N) {
+      Object.setPrototypeOf(arguments, proto);
+    }
+
+    var arg = arguments[0];
+
+    assertEq(arg, i !== N ? undefined : "pass");
+  }
+}
+outOfBounds();

js/src/jit-test/tests/arguments/args-mutate-proto-2.js

@@ -0,0 +1,16 @@
+function outOfBounds() {
+  var proto = [];
+  Object.setPrototypeOf(arguments, proto);
+
+  var N = 100;
+  for (var i = 0; i <= N; ++i) {
+    if (i === N) {
+      proto[0] = "pass";
+    }
+
+    var arg = arguments[0];
+
+    assertEq(arg, i !== N ? undefined : "pass");
+  }
+}
+outOfBounds();

js/src/jit-test/tests/arguments/args-mutate-proto-3.js

@@ -0,0 +1,15 @@
+function outOfBounds() {
+  Object.setPrototypeOf(arguments, Array.prototype);
+
+  var N = 100;
+  for (var i = 0; i <= N; ++i) {
+    if (i === N) {
+      Array.prototype[0] = "pass";
+    }
+
+    var arg = arguments[0];
+
+    assertEq(arg, i !== N ? undefined : "pass");
+  }
+}
+outOfBounds();

js/src/jit-test/tests/arguments/args-mutate-proto-4.js

@@ -0,0 +1,13 @@
+function outOfBounds() {
+  var N = 100;
+  for (var i = 0; i <= N; ++i) {
+    if (i === N) {
+      Object.prototype[0] = "pass";
+    }
+
+    var arg = arguments[0];
+
+    assertEq(arg, i !== N ? undefined : "pass");
+  }
+}
+outOfBounds();

js/src/jit/CacheIR.cpp

@@ -416,6 +416,7 @@ AttachDecision GetPropIRGenerator::tryAttachStub() {
       TRY_ATTACH(tryAttachDenseElementHole(obj, objId, index, indexId));
       TRY_ATTACH(tryAttachSparseElement(obj, objId, index, indexId));
       TRY_ATTACH(tryAttachArgumentsObjectArg(obj, objId, index, indexId));
+      TRY_ATTACH(tryAttachArgumentsObjectArgHole(obj, objId, index, indexId));
       TRY_ATTACH(tryAttachGenericElement(obj, objId, index, indexId));
 
       trackAttached(IRGenerator::NotAttached);
@@ -2262,7 +2263,8 @@ static bool ClassCanHaveExtraProperties(const JSClass* clasp) {
 }
 
 static bool CanAttachDenseElementHole(NativeObject* obj, bool ownProp,
-                                      bool allowIndexedReceiver = false) {
+                                      bool allowIndexedReceiver = false,
+                                      bool allowExtraProperties = false) {
   // Make sure the objects on the prototype don't have any indexed properties
   // or that such properties can't appear without a shape change.
   // Otherwise returning undefined for holes would obviously be incorrect,
@@ -2274,9 +2276,10 @@ static bool CanAttachDenseElementHole(NativeObject* obj, bool ownProp,
     }
     allowIndexedReceiver = false;
 
-    if (ClassCanHaveExtraProperties(obj->getClass())) {
+    if (!allowExtraProperties && ClassCanHaveExtraProperties(obj->getClass())) {
       return false;
     }
+    allowExtraProperties = false;
 
     // Don't need to check prototype for OwnProperty checks
     if (ownProp) {
@@ -2340,6 +2343,51 @@ AttachDecision GetPropIRGenerator::tryAttachArgumentsObjectArg(
   return AttachDecision::Attach;
 }
 
+AttachDecision GetPropIRGenerator::tryAttachArgumentsObjectArgHole(
+    HandleObject obj, ObjOperandId objId, uint32_t index,
+    Int32OperandId indexId) {
+  if (!obj->is<ArgumentsObject>()) {
+    return AttachDecision::NoAction;
+  }
+  auto* args = &obj->as<ArgumentsObject>();
+
+  // No elements must have been overridden or deleted.
+  if (args->hasOverriddenElement()) {
+    return AttachDecision::NoAction;
+  }
+
+  // And also check that the argument isn't forwarded.
+  if (index < args->initialLength() && args->argIsForwarded(index)) {
+    return AttachDecision::NoAction;
+  }
+
+  if (!CanAttachDenseElementHole(args, false, true, true)) {
+    return AttachDecision::NoAction;
+  }
+
+  // We don't need to guard on the shape, because we check if any element is
+  // overridden. Elements are marked as overridden iff any element is defined,
+  // irrespective of whether the element is in-bounds or out-of-bounds. So when
+  // that flag isn't set, we can guarantee that the arguments object doesn't
+  // have any additional own elements.
+
+  if (args->is<MappedArgumentsObject>()) {
+    writer.guardClass(objId, GuardClassKind::MappedArguments);
+  } else {
+    MOZ_ASSERT(args->is<UnmappedArgumentsObject>());
+    writer.guardClass(objId, GuardClassKind::UnmappedArguments);
+  }
+
+  GeneratePrototypeHoleGuards(writer, args, objId,
+                              /* alwaysGuardFirstProto = */ true);
+
+  writer.loadArgumentsObjectArgHoleResult(objId, indexId);
+  writer.returnFromIC();
+
+  trackAttached("ArgumentsObjectArgHole");
+  return AttachDecision::Attach;
+}
+
 AttachDecision GetPropIRGenerator::tryAttachArgumentsObjectCallee(
     HandleObject obj, ObjOperandId objId, HandleId id) {
   // Only mapped arguments objects have a `callee` property.

js/src/jit/CacheIR.h

@@ -1329,6 +1329,10 @@ class MOZ_RAII GetPropIRGenerator : public IRGenerator {
   AttachDecision tryAttachArgumentsObjectArg(HandleObject obj,
                                              ObjOperandId objId, uint32_t index,
                                              Int32OperandId indexId);
+  AttachDecision tryAttachArgumentsObjectArgHole(HandleObject obj,
+                                                 ObjOperandId objId,
+                                                 uint32_t index,
+                                                 Int32OperandId indexId);
   AttachDecision tryAttachArgumentsObjectCallee(HandleObject obj,
                                                 ObjOperandId objId,
                                                 HandleId id);

js/src/jit/CacheIRCompiler.cpp

@@ -3468,6 +3468,24 @@ bool CacheIRCompiler::emitLoadArgumentsObjectArgResult(ObjOperandId objId,
   return true;
 }
 
+bool CacheIRCompiler::emitLoadArgumentsObjectArgHoleResult(
+    ObjOperandId objId, Int32OperandId indexId) {
+  JitSpew(JitSpew_Codegen, "%s", __FUNCTION__);
+  AutoOutputRegister output(*this);
+  Register obj = allocator.useRegister(masm, objId);
+  Register index = allocator.useRegister(masm, indexId);
+  AutoScratchRegister scratch(allocator, masm);
+
+  FailurePath* failure;
+  if (!addFailurePath(&failure)) {
+    return false;
+  }
+
+  masm.loadArgumentsObjectElementHole(obj, index, output.valueReg(), scratch,
+                                      failure->label());
+  return true;
+}
+
 bool CacheIRCompiler::emitLoadDenseElementResult(ObjOperandId objId,
                                                  Int32OperandId indexId) {
   JitSpew(JitSpew_Codegen, "%s", __FUNCTION__);

js/src/jit/CacheIROps.yaml

@@ -1797,6 +1797,14 @@
     obj: ObjId
     index: Int32Id
 
+- name: LoadArgumentsObjectArgHoleResult
+  shared: true
+  transpile: false
+  cost_estimate: 2
+  args:
+    obj: ObjId
+    index: Int32Id
+
 - name: LoadArgumentsObjectLengthResult
   shared: true
   transpile: true

js/src/jit/MacroAssembler.cpp

@@ -4382,6 +4382,41 @@ void MacroAssembler::loadArgumentsObjectElement(Register obj, Register index,
   loadValue(argValue, output);
 }
 
+void MacroAssembler::loadArgumentsObjectElementHole(Register obj,
+                                                    Register index,
+                                                    ValueOperand output,
+                                                    Register temp,
+                                                    Label* fail) {
+  Register temp2 = output.scratchReg();
+
+  // Get initial length value.
+  unboxInt32(Address(obj, ArgumentsObject::getInitialLengthSlotOffset()), temp);
+
+  // Ensure no overridden elements.
+  branchTest32(Assembler::NonZero, temp,
+               Imm32(ArgumentsObject::ELEMENT_OVERRIDDEN_BIT), fail);
+
+  // Bounds check.
+  Label outOfBounds, done;
+  rshift32(Imm32(ArgumentsObject::PACKED_BITS_COUNT), temp);
+  spectreBoundsCheck32(index, temp, temp2, &outOfBounds);
+
+  // Load ArgumentsData.
+  loadPrivate(Address(obj, ArgumentsObject::getDataSlotOffset()), temp);
+
+  // Guard the argument is not a FORWARD_TO_CALL_SLOT MagicValue.
+  BaseValueIndex argValue(temp, index, ArgumentsData::offsetOfArgs());
+  branchTestMagic(Assembler::Equal, argValue, fail);
+  loadValue(argValue, output);
+  jump(&done);
+
+  bind(&outOfBounds);
+  branch32(Assembler::LessThan, index, Imm32(0), fail);
+  moveValue(UndefinedValue(), output);
+
+  bind(&done);
+}
+
 void MacroAssembler::loadArgumentsObjectLength(Register obj, Register output,
                                                Label* fail) {
   // Get initial length value.

js/src/jit/MacroAssembler.h

@@ -4787,6 +4787,9 @@ class MacroAssembler : public MacroAssemblerSpecific {
   void loadArgumentsObjectElement(Register obj, Register index,
                                   ValueOperand output, Register temp,
                                   Label* fail);
+  void loadArgumentsObjectElementHole(Register obj, Register index,
+                                      ValueOperand output, Register temp,
+                                      Label* fail);
 
   void loadArgumentsObjectLength(Register obj, Register output, Label* fail);