comms doesn't set up reverse mapping for promises correctly

[warner]

Describe the bug

While investigating #1400, I noticed a problem in mapInbound. For objects, we correctly populate the per-remote table with to/from entries that flip the remote identifier's ownership polarity:

agoric-sdk/packages/SwingSet/src/vats/comms/clist.js

Lines 174 to 187 in 1685098

	if (allocatedByRecipient) {
	throw new Error(`I don't remember giving ${s} to ${remoteID}`);
	}
	// this must be a new import. Allocate a new vat object for it, which
	// will be the local machine's proxy for use by all other local vats
	const localSlot = makeVatSlot(type, true, state.nextObjectIndex);
	state.nextObjectIndex += 1;
	// they sent us ro-NN
	remote.fromRemote.set(s, localSlot);
	// when we send it back, we'll send ro+NN
	remote.toRemote.set(localSlot, flipRemoteSlot(s));
	// and remember how to route this later
	state.objectTable.set(localSlot, remoteID);

to honor the specification:

agoric-sdk/packages/SwingSet/src/vats/comms/remote.js

Lines 27 to 40 in 1685098

	// The keys of the fromRemote table will have the opposite allocator flag
	// as the corresponding value of the toRemote table. The only time we must
	// reverse the polarity of the flag is when we add a new entry to the
	// clist.
	// fromRemote has:
	// ro-NN -> o+NN (imported/importing from remote machine)
	// ro+NN -> o-NN (previously exported to remote machine)
	const fromRemote = new Map(); // {ro/rp/rr+-NN} -> o+-NN/p+-NN/r+-NN
	// toRemote has:
	// o+NN -> ro+NN (previously imported from remote machine)
	// o-NN -> ro-NN (exported/exporting to remote machine)
	const toRemote = new Map(); // o/p/r+-NN -> ro/rp/rr+-NN

however when we allocate a new promise, we fail to flip the toRemote side:

agoric-sdk/packages/SwingSet/src/vats/comms/clist.js

Lines 188 to 196 in 1685098

	} else if (type === 'promise') {
	if (allocatedByRecipient) {
	throw new Error(`I don't remember giving ${s} to ${remoteID}`);
	} else {
	const promiseID = allocateUnresolvedPromise(state, remoteID);
	remote.fromRemote.set(s, promiseID);
	remote.toRemote.set(promiseID, s);
	// console.debug(`inbound promise ${s} mapped to ${promiseID}`);
	}

When the inbound promise appeared as a result, we fix it up in mapInboundResult:

agoric-sdk/packages/SwingSet/src/vats/comms/clist.js

Lines 214 to 228 in 1685098

	export function mapInboundResult(state, remoteID, result) {
	insistRemoteType('promise', result);
	assert(!parseRemoteSlot(result).allocatedByRecipient, details`${result}`); // temp?
	const r = mapInbound(state, remoteID, result);
	insistVatType('promise', r);
	insistPromiseIsUnresolved(state, r);
	insistPromiseDeciderIs(state, r, remoteID);
	insistPromiseSubscriberIsNotDifferent(state, r, remoteID);
	setPromiseDecider(state, r, null); // (local kernel / other local vat) now decides
	setPromiseSubscriber(state, r, remoteID); // auto-subscribe the sender
	const remote = getRemote(state, remoteID);
	remote.fromRemote.set(result, r);
	remote.toRemote.set(r, flipRemoteSlot(result));
	return r;
	}

but we'll fail to do that for promises that appear anywhere else, like in arguments of an inbound message, or in a record used to resolve some other promise to data.

The consequence is that when we try to send these objects back to the sender, we'll give them an identifier with the wrong polarity. At best, they'll refuse the message because they don't recognize the promise ID. At worst, it will map to some unrelated promise, which would be an exciting security bug.

TODO items for me:

• add test (to message-patterns, the "comms" flavor) of a new argument promise being sent back to the sender
• add test of a new resolve-to-data promise being sent back to the sender
• change mapInbound to flip the direction of the remote identifier we use in toRemote
• change mapInboundResult to not modify toRemote/fromRemote at all

[warner]

I ended up fixing this (and a slew of other problems) by rewriting the comms vat entirely, in PR #1635