fix(marshal)!: Update compareRank to terminate comparability at the f…

…irst remotable (#2597)

Fixes #2588

To be refinable into a total order that distinguishes remotables,
`compareRank` must consider `[r1, 'x']` vs. `[r2, 'y']` as a tie rather
than as equivalent to `[0, 'x']` vs. `[0, 'y']`, in case r1 vs. r2 ends
up comparing differently than 'x' vs. 'y'.

packages/marshal/NEWS.md

@@ -1,5 +1,9 @@
 User-visible changes in `@endo/marshal`:
 
+# Next release
+
+- `compareRank` now short-circuits upon encountering remotables to compare, considering the inputs to be tied for the same rank regardless of what would otherwise be visited later in their respective data structures. This ensures that a `fullCompare` which does distinguish remotables will be a refinement of `compareRank`, rather than disagreeing about whether or not two values share a rank ([#2588](https://github.com/endojs/endo/issues/2588)).
+
 # v1.5.1 (2024-07-30)
 
 - `deeplyFulfilled` moved from @endo/marshal to @endo/pass-style. @endo/marshal still reexports it, to avoid breaking old importers. But importers should be upgraded to import `deeplyFulfilled` directly from @endo/pass-style.

packages/marshal/src/encodePassable.js

@@ -877,6 +877,9 @@ export const passStylePrefixes = {
   string: 's',
   null: 'v',
   symbol: 'y',
+  // Because Array.prototype.sort puts undefined values at the end without
+  // passing them to a comparison function, undefined MUST be the last
+  // category.
   undefined: 'z',
 };
 Object.setPrototypeOf(passStylePrefixes, null);

packages/marshal/src/rankOrder.js

@@ -8,7 +8,7 @@ import {
 
 /**
  * @import {Passable, PassStyle} from '@endo/pass-style'
- * @import {FullCompare, RankCompare, RankCover} from './types.js'
+ * @import {FullCompare, PartialCompare, PartialComparison, RankCompare, RankCover} from './types.js'
  */
 
 const { entries, fromEntries, setPrototypeOf, is } = Object;
@@ -101,15 +101,16 @@ const memoOfSorted = new WeakMap();
 const comparatorMirrorImages = new WeakMap();
 
 /**
- * @param {RankCompare=} compareRemotables
- * An option to create a comparator in which an internal order is
- * assigned to remotables. This defaults to a comparator that
- * always returns `0`, meaning that all remotables are tied
- * for the same rank.
+ * @param {PartialCompare} [compareRemotables]
+ * A comparator for assigning an internal order to remotables.
+ * It defaults to a function that always returns `NaN`, meaning that all
+ * remotables are incomparable and should tie for the same rank by
+ * short-circuiting without further refinement (e.g., not only are `r1` and `r2`
+ * tied, but so are `[r1, 0]` and `[r2, "x"]`).
  * @returns {RankComparatorKit}
  */
-export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => {
-  /** @type {RankCompare} */
+export const makeComparatorKit = (compareRemotables = (_x, _y) => NaN) => {
+  /** @type {PartialCompare} */
   const comparator = (left, right) => {
     if (sameValueZero(left, right)) {
       return 0;
@@ -191,10 +192,9 @@ export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => {
         if (result !== 0) {
           return result;
         }
-        return comparator(
-          recordValues(left, leftNames),
-          recordValues(right, rightNames),
-        );
+        const leftValues = recordValues(left, leftNames);
+        const rightValues = recordValues(right, rightNames);
+        return comparator(leftValues, rightValues);
       }
       case 'copyArray': {
         // Lexicographic
@@ -225,14 +225,20 @@ export const makeComparatorKit = (compareRemotables = (_x, _y) => 0) => {
   };
 
   /** @type {RankCompare} */
-  const antiComparator = (x, y) => comparator(y, x);
+  const outerComparator = (x, y) =>
+    // When the inner comparator returns NaN to indicate incomparability,
+    // replace that with 0 to indicate a tie.
+    /** @type {Exclude<PartialComparison, NaN>} */ (comparator(x, y) || 0);
+
+  /** @type {RankCompare} */
+  const antiComparator = (x, y) => outerComparator(y, x);
 
-  memoOfSorted.set(comparator, new WeakSet());
+  memoOfSorted.set(outerComparator, new WeakSet());
   memoOfSorted.set(antiComparator, new WeakSet());
-  comparatorMirrorImages.set(comparator, antiComparator);
-  comparatorMirrorImages.set(antiComparator, comparator);
+  comparatorMirrorImages.set(outerComparator, antiComparator);
+  comparatorMirrorImages.set(antiComparator, outerComparator);
 
-  return harden({ comparator, antiComparator });
+  return harden({ comparator: outerComparator, antiComparator });
 };
 /**
  * @param {RankCompare} comparator
@@ -275,15 +281,6 @@ export const assertRankSorted = (sorted, compare) =>
 harden(assertRankSorted);
 
 /**
- * TODO SECURITY BUG: https://github.com/Agoric/agoric-sdk/issues/4260
- * sortByRank currently uses `Array.prototype.sort` directly, and
- * so only works correctly when given a `compare` function that considers
- * `undefined` strictly bigger (`>`) than everything else. This is
- * because `Array.prototype.sort` bizarrely moves all `undefined`s to
- * the end of the array regardless, without consulting the `compare`
- * function. This is a genuine bug for us NOW because sometimes we sort
- * in reverse order by passing a reversed rank comparison function.
- *
  * @template {Passable} T
  * @param {Iterable<T>} passables
  * @param {RankCompare} compare
@@ -301,7 +298,20 @@ export const sortByRank = (passables, compare) => {
   }
   const unsorted = [...passables];
   unsorted.forEach(harden);
-  const sorted = harden(unsorted.sort(compare));
+  const sorted = unsorted.sort(compare);
+  // For reverse comparison, move `undefined` values from the end to the start.
+  // Note that passStylePrefixes (@see {@link ./encodePassable.js}) MUST NOT
+  // sort any category after `undefined`.
+  if (compare(true, undefined) > 0) {
+    let i = sorted.length - 1;
+    while (i >= 0 && sorted[i] === undefined) i -= 1;
+    const n = sorted.length - i - 1;
+    if (n > 0 && n < sorted.length) {
+      sorted.copyWithin(n, 0);
+      sorted.fill(/** @type {T} */ (undefined), 0, n);
+    }
+  }
+  harden(sorted);
   const subMemoOfSorted = memoOfSorted.get(compare);
   assert(subMemoOfSorted !== undefined);
   subMemoOfSorted.add(sorted);

packages/marshal/src/types.js

@@ -156,45 +156,66 @@ export {};
  * Returns `-1`, `0`, or `1` depending on whether the rank of `left`
  * is respectively before, tied-with, or after the rank of `right`.
  *
- * This comparison function is valid as argument to
- * `Array.prototype.sort`. This is sometimes described as a "total order"
- * but, depending on your definitions, this is technically incorrect because
- * it may return `0` to indicate that two distinguishable elements such as
- * `-0` and `0` are tied (i.e., are in the same equivalence class
- * for the purposes of this ordering). If each such equivalence class is
- * a *rank* and ranks are disjoint, then this "rank order" is a
- * true total order over these ranks. In mathematics this goes by several
- * other names such as "total preorder".
+ * As a total preorder, this comparison function is valid as an argument to
+ * `Array.prototype.sort` but may return `0` to indicate that two
+ * distinguishable elements such as `-0` and `0` are tied (i.e., are in the same
+ * equivalence class for the purposes of this ordering). If each such
+ * equivalence class is
+ * a *rank* and ranks are disjoint, then this "rank order" is a true total order
+ * over these ranks.
  *
  * This function establishes a total rank order over all passables.
  * To do so it makes arbitrary choices, such as that all strings
- * are after all numbers. Thus, this order is not intended to be
- * used directly as a comparison with useful semantics. However, it must be
- * closely enough related to such comparisons to aid in implementing
- * lookups based on those comparisons. For example, in order to get a total
- * order among ranks, we put `NaN` after all other JavaScript "number" values
- * (i.e., IEEE 754 floating-point values). But otherwise, we rank JavaScript
- * numbers by signed magnitude, with `0` and `-0` tied. A semantically useful
- * ordering would also compare magnitudes, and so agree with the rank ordering
- * of all values other than `NaN`. An array sorted by rank would enable range
- * queries by magnitude.
+ * are after all numbers, and thus is not intended to be used directly as a
+ * comparison with useful semantics.
+ * However, it must be closely enough related to such comparisons to aid in
+ * implementing lookups based on those comparisons.
+ * For example, in order to get a total order over ranks, we put `NaN` after all
+ * other JavaScript "number" values (i.e., IEEE 754 floating-point values) but
+ * otherwise rank JavaScript numbers by signed magnitude, with `0` and `-0`
+ * tied, as would a semantically useful ordering such as `KeyCompare` in
+ * {@link ../../patterns}.
+ * Likewise, an array sorted by rank would enable range queries by magnitude.
+ *
  * @param {any} left
  * @param {any} right
  * @returns {RankComparison}
  */
 
 /**
  * @typedef {RankCompare} FullCompare
- * A `FullCompare` function satisfies all the invariants stated below for
- * `RankCompare`'s relation with KeyCompare.
- * In addition, its equality is as precise as the `KeyCompare`
- * comparison defined below, in that, for all Keys `x` and `y`,
- * `FullCompare(x, y) === 0` iff `KeyCompare(x, y) === 0`.
+ * A function that refines `RankCompare` into a total order over its inputs by
+ * making arbitrary choices about the relative ordering of values within the
+ * same rank.
+ * Like `RankCompare` but even more strongly, it is expected to agree with a
+ * `KeyCompare` (@see {@link ../../patterns}) where they overlap ---
+ * `FullCompare(key1, key2) === 0` iff `KeyCompare(key1, key2) === 0`.
+ */
+
+/**
+ * @typedef {-1 | 0 | 1 | NaN} PartialComparison
+ * The result of a `PartialCompare` function that defines a meaningful and
+ * meaningfully precise partial order in which incomparable values are
+ * represented by `NaN`. See `PartialCompare`.
+ */
+
+/**
+ * @template [T=any]
+ * @callback PartialCompare
+ * A function that implements a partial order --- defining relative position
+ * between values but leaving some pairs incomparable (for example, subsets over
+ * sets is a partial order in which {} precedes {x} and {y}, which are mutually
+ * incomparable but both precede {x, y}). As with the rank ordering produced by
+ * `RankCompare`, -1, 0, and 1 respectively mean "less than", "equivalent to",
+ * and "greater than". NaN means "incomparable" --- the first value is not less,
+ * equivalent, or greater than the second.
+ *
+ * By using NaN for "incomparable", the normal equivalence for using
+ * the return value in a comparison is preserved.
+ * `PartialCompare(left, right) >= 0` iff `left` is greater than or equivalent
+ * to `right` in the partial ordering.
  *
- * For non-keys a `FullCompare` should be exactly as imprecise as
- * `RankCompare`. For example, both will treat all errors as in the same
- * equivalence class. Both will treat all promises as in the same
- * equivalence class. Both will order taggeds the same way, which is admittedly
- * weird, as some taggeds will be considered keys and other taggeds will be
- * considered non-keys.
+ * @param {T} left
+ * @param {T} right
+ * @returns {PartialComparison}
  */

packages/marshal/test/_marshal-test-data.js

@@ -271,6 +271,9 @@ export const unsortedSample = harden([
   Symbol.for(''),
   false,
   exampleCarol,
+  [exampleCarol, 'm'],
+  [exampleAlice, 'a'],
+  [exampleBob, 'z'],
   -0,
   {},
   [5, undefined],
@@ -350,6 +353,9 @@ export const sortedSample = harden([
   [5, { foo: 4, bar: undefined }],
   [5, null],
   [5, undefined],
+  [exampleAlice, 'a'],
+  [exampleCarol, 'm'],
+  [exampleBob, 'z'],
 
   false,
   true,