[ubsan] Fix two more UBSan issues

RotateRight32 needs a "number of bits" operand in the range 0..31.
Thankfully that's how x86 shift instructions behave anyway, and
how the bitwise shift operators in JavaScript are spec'ed, so this
fix is unobservable in non-UBSan builds.

RemoveArrayHolesGeneric can be used for length values anywhere in
the uint32_t range, so it must not implicitly cast those to int.
That actually caused an observable bug where a proxy's traps would
not get called at all, but only for huge "length" properties, where
the entire operation would also be painfully slow.

Bug: chromium:935133, chromium:937652
Change-Id: I13f74ca27eae6b2b089d58217842b699b2574509
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1510272
Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60112}

src/compiler/machine-operator-reducer.cc

@@ -173,8 +173,8 @@ Reduction MachineOperatorReducer::Reduce(Node* node) {
       Int32BinopMatcher m(node);
       if (m.right().Is(0)) return Replace(m.left().node());  // x ror 0 => x
       if (m.IsFoldable()) {                                  // K ror K => K
-        return ReplaceInt32(
-            base::bits::RotateRight32(m.left().Value(), m.right().Value()));
+        return ReplaceInt32(base::bits::RotateRight32(m.left().Value(),
+                                                      m.right().Value() & 31));
       }
       break;
     }

src/runtime/runtime-array.cc

@@ -80,7 +80,7 @@ Object RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
 
   uint32_t num_undefined = 0;
   uint32_t current_pos = 0;
-  int num_indices = keys.is_null() ? limit : keys->length();
+  uint32_t num_indices = keys.is_null() ? limit : keys->length();
 
   // Compact keys with undefined values and moves non-undefined
   // values to the front.
@@ -91,7 +91,7 @@ Object RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
   //       is used to track free spots in the array starting at the beginning.
   //       Holes and 'undefined' are considered free spots.
   //       A hole is when HasElement(receiver, key) is false.
-  for (int i = 0; i < num_indices; ++i) {
+  for (uint32_t i = 0; i < num_indices; ++i) {
     uint32_t key = keys.is_null() ? i : NumberToUint32(keys->get(i));
 
     // We only care about array indices that are smaller than the limit.
@@ -163,7 +163,8 @@ Object RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
   // DCHECK_LE(current_pos, num_indices);
 
   // Deleting everything after the undefineds up unto the limit.
-  for (int i = num_indices - 1; i >= 0; --i) {
+  for (uint32_t i = num_indices; i > 0;) {
+    --i;
     uint32_t key = keys.is_null() ? i : NumberToUint32(keys->get(i));
     if (key < current_pos) break;
     if (key >= limit) continue;

test/mjsunit/ubsan-fuzzerbugs.js

@@ -54,3 +54,39 @@ float_array[0] = 1e51;
   %OptimizeFunctionOnNextCall(__f_14159);
   __f_14159(buffer);
 })();
+
+// crbug.com/937652
+(function() {
+  function f() {
+    for (var i = 0; i < 1; i++) {
+      var shift = 1;
+      for (var j = 0; j < 2; ++j) {
+        if (shift == shift) shift = 0;
+        var x = 1;
+        print((x << shift | x >>> 32 - shift));
+      }
+    }
+  }
+  f();
+  f();
+  %OptimizeFunctionOnNextCall(f);
+  f();
+})();
+
+// crbug.com/935133
+(function() {
+  var called_has = false;
+  var proxy = new Proxy({}, {
+    has: function(x, p) {
+      called_has = true;
+      throw "The test may finish now";
+    },
+  });
+  proxy.length = 2147483648;
+  try {
+    Array.prototype.sort.call(proxy);
+  } catch(e) {
+    assertTrue(e === "The test may finish now");
+  }
+  assertTrue(called_has);
+})();