fix: disable comms vat termination via remote comms errors

Agoric · May 4, 2021 · d286fbd · d286fbd
1 parent 6a1705e
commit d286fbd
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 28 deletions.
diff --git a/packages/SwingSet/src/vats/comms/dispatch.js b/packages/SwingSet/src/vats/comms/dispatch.js
@@ -72,11 +72,38 @@ export function buildCommsDispatch(
 
     const remoteID = state.getRemoteReceiver(target);
     if (remoteID) {
-      assert(method === 'receive', X`unexpected method ${method}`);
-      // the vat-tp integrity layer is a regular vat, so when they send the
-      // received message to us, it will be embedded in a JSON array
-      const message = JSON.parse(args.body)[0];
-      return messageFromRemote(remoteID, message, result);
+      // XXX TODO DANGER DANGER WARNING DANGER VERY BAD BADNESS FIX ASAP OH NOES
+      // The following try/catch intercepts errors triggered by badly
+      // constructed or badly parameterized messages from the remote, and logs
+      // them instead of allowing them to propagate upward and kill the vat.
+      // Ideally, such conditions should just kill the connection to the remote,
+      // but we don't yet have the means set up to do that kind of selective
+      // disablement.  MORE IMPORTANTLY, HOWEVER, this try/catch will actually
+      // intercept ALL errors generated in the processing of the remote message,
+      // including ones that aren't the remote's fault and including ones that
+      // really should kill the vat.  Unfortunately, pretty much all such error
+      // checking is currently done via calls to `assert` or one of its
+      // siblings, which doesn't distinguish the appropriate scope of punishment
+      // for any transgression.  This blind sledgehammer approach needs to be
+      // replaced with a more discriminating mechanism.  Until that is done, the
+      // comms vat remains at risk of inconsistency due to internal errors.
+      // Moreover, if an individual remote connection gets into an inconsistent
+      // state due to misbehavior on the other end, we have no way to kill that
+      // connection and thus we risk whatever weirdness may ensue from the
+      // remote continuing to think everything's ok (and behaving accordingly)
+      // when it's not.  All of this should be fixed soon as practicable.
+      // DANGER WILL ROBINSON CASE NIGHTMARE GREEN ELDRITCH HORRORS OH THE HUMANITY
+      try {
+        assert(method === 'receive', X`unexpected method ${method}`);
+        // the vat-tp integrity layer is a regular vat, so when they send the
+        // received message to us, it will be embedded in a JSON array
+        const message = JSON.parse(args.body)[0];
+        return messageFromRemote(remoteID, message, result);
+      } catch (err) {
+        console.log('WARNING: delivery from remote triggered error:', err);
+        console.log(`Error happened while processing "${args.body}"`);
+        return undefined;
+      }
     }
 
     // If we get to this point, the message is not being delivered to a

diff --git a/packages/SwingSet/test/test-comms.js b/packages/SwingSet/test/test-comms.js
@@ -220,19 +220,21 @@ test('receive', t => {
   ]);
 
   // react to bad sequence number
-  t.throws(
-    () =>
-      dispatch(
-        makeMessage(
-          receiverID,
-          'receive',
-          encodeArgs(
-            `47:deliver:${bobRemote}:bar::ro-20:${bobRemote};argsbytes`,
-          ),
-        ),
-      ),
-    { message: /unexpected recv seqNum .*/ },
-  );
+  // THIS CHECK TEMPORARILY DISABLED DUE TO THE SUPPRESION OF REMOTE COMMS ERRORS
+  // See issue #3021
+  // t.throws(
+  //   () =>
+  //     dispatch(
+  //       makeMessage(
+  //         receiverID,
+  //         'receive',
+  //         encodeArgs(
+  //           `47:deliver:${bobRemote}:bar::ro-20:${bobRemote};argsbytes`,
+  //         ),
+  //       ),
+  //     ),
+  //   { message: /unexpected recv seqNum .*/ },
+  // );
 
   // make sure comms can tolerate GC operations, even if they're a no-op
   dispatch(makeDropExports(expectedAliceKernel, expectedAyanaKernel));
@@ -630,16 +632,18 @@ test('outbound promise resolution and inbound message to it crossing in flight',
   // Promise should still be in the comms vat, because no ack yet
   t.is(state.getPromiseStatus(plPromise), 'fulfilled');
 
-  _('a:l', 0); // lag ends, talking to the now retired promise should error
-  t.throws(
-    () => _('a>m', paPromise, 'talkback', undefined),
-    { message: `"${refOf(paPromise)}" must already be in remote "r1 (a)"` },
-  );
-
-  // Alice should be able to address Lisa directly & the promise should be gone
-  _('a>m', oaLisa, 'moretalk', undefined);
-  _('k<m', oLisa, 'moretalk', undefined);
-  t.is(state.getPromiseStatus(plPromise), undefined);
+  // THIS CHECK TEMPORARILY DISABLED DUE TO THE SUPPRESION OF REMOTE COMMS ERRORS
+  // See issue #3021
+  // _('a:l', 0); // lag ends, talking to the now retired promise should error
+  // t.throws(
+  //   () => _('a>m', paPromise, 'talkback', undefined),
+  //   { message: `"${refOf(paPromise)}" must already be in remote "r1 (a)"` },
+  // );
+
+  // // Alice should be able to address Lisa directly & the promise should be gone
+  // _('a>m', oaLisa, 'moretalk', undefined);
+  // _('k<m', oLisa, 'moretalk', undefined);
+  // t.is(state.getPromiseStatus(plPromise), undefined);
 
   done();
 });