Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: triage await safety for vats,xsnap #6531

Closed
wants to merge 1 commit into from
Closed

fix: triage await safety for vats,xsnap #6531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/vats/src/core/chain-behaviors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,12 @@ export const registerNetworkProtocols = async (vats, dibcBridgeManager) => {
});
},
});
// This nested await is safe because "terminal-control-flow".
//
// It occurs at the top level of one branch of an unbalanced non-top-level
// if. However, immediately after the if is another await expression
// that has no effects prior to the turn boundary.
// eslint-disable-next-line @jessie.js/no-nested-await
const ibcHandler = await E(vats.ibc).createInstance(callbacks);
dibcBridgeManager.register(BRIDGE_ID.DIBC, ibcHandler);
ps.push(
Expand Down
13 changes: 13 additions & 0 deletions packages/vats/src/ibc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,13 @@ export function makeIBCProtocolHandler(E, rawCallIBCDevice) {
const attemptP = E(protocolImpl).inbound(localAddr, remoteAddr);

// Tell what version string we negotiated.
//
// This nested await is safe because "terminal-control-flow".
//
// It occurs at the top level of one branch of an unbalanced, but
// top level and terminal switch statement. Nothing happens after
// the switch.
// eslint-disable-next-line @jessie.js/no-nested-await
const attemptedLocal = await E(attemptP).getLocalAddress();
const match = attemptedLocal.match(
// Match: ... /ORDER/VERSION ...
Expand Down Expand Up @@ -493,6 +500,9 @@ export function makeIBCProtocolHandler(E, rawCallIBCDevice) {
rPortID,
order,
);
// This nested await is safe because "terminal-control-flow".
//
// eslint-disable-next-line @jessie.js/no-nested-await
const localAddr = await E(attemptP).getLocalAddress();
E(attemptP).accept({
localAddress: `${localAddr}/ibc-channel/${channelID}`,
Expand All @@ -512,6 +522,9 @@ export function makeIBCProtocolHandler(E, rawCallIBCDevice) {
const connP = channelKeyToConnP.get(channelKey);
const data = base64ToBytes(data64);

// This nested await is safe because "terminal-control-flow".
//
// eslint-disable-next-line @jessie.js/no-nested-await
await E(connP)
.send(data)
.then(ack => {
Expand Down
4 changes: 3 additions & 1 deletion packages/vats/src/vat-bank.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ const makePurseController = (
let updateRecord = await balanceNotifier.getUpdateSince();
while (updateRecord.updateCount) {
yield updateRecord.value;
// eslint-disable-next-line no-await-in-loop
// This nested await is safe because "terminal-control-flow".
//
// eslint-disable-next-line no-await-in-loop, @jessie.js/no-nested-await
updateRecord = await balanceNotifier.getUpdateSince(
updateRecord.updateCount,
);
Expand Down
72 changes: 61 additions & 11 deletions packages/xsnap/src/replay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,11 @@ export async function replayXSnap(
*/
async function runSteps(rd, steps) {
const folder = rd.path;
for (const step of steps) {
// Changed from `for of` to `for await of` to make awaits within
// loop body safer. A `for await of` will correctly process a synchronous
// iterator, but it still unconditionally separates the iterations
// with turn boundaries.
for await (const step of steps) {
const parts = step.match(/(\d+)-([a-zA-Z]+)\.(dat|json)$/);
if (!parts) {
throw Error(`expected 0001-abc.dat; got: ${step}`);
Expand All @@ -217,45 +221,86 @@ export async function replayXSnap(
const seq = parseInt(digits, 10);
console.log(folder, seq, kind);
if (running && !['command', 'reply'].includes(kind)) {
// eslint-disable-next-line no-await-in-loop
// This nested await is safe because "all-branches-balanced".
//
// This nested synchronous-throw-impossible await at the top level of
// the then branch is safe because it is balanced by a
// synchronous-throw-impossible await at the top level of the else
// branch. In fact the await in the else branch was introduced for
// that balance, in order to make this one safe.
// eslint-disable-next-line @jessie.js/no-nested-await
await running;
running = undefined;
} else {
// This nested await is safe because "all-branches-balanced".
//
// This nested synchronous-throw-impossible await at the top level of
// the else branch is safe because it is balanced by a
// synchronous-throw-impossible await at the top level of the then
// branch. In fact it was introduced for that balance, in order to
// make the `await` in the then branch safe.
// eslint-disable-next-line @jessie.js/no-nested-await
await null;
}
const file = rd.file(step);
switch (kind) {
case 'isReady':
// eslint-disable-next-line no-await-in-loop
case 'isReady': {
// This nested await is safe because "terminal-control-flow".
//
// The awaits within the switch are unbalanced. The occur on some
// branches and not others. This would be ok if the switch
// were terminal control flow, i.e., if no stateful synchronous code
// happens after the switch. This is now true, because of the
// extra `await null` turn boundary before the remaining stateful
// code. Now that code will always happen after a turn boundary.
//
// eslint-disable-next-line @jessie.js/no-nested-await
await it.isReady();
break;
case 'evaluate':
}
case 'evaluate': {
running = it.evaluate(file.getText());
break;
case 'issueCommand':
}
case 'issueCommand': {
running = it.issueCommand(file.getData());
break;
case 'command':
}
case 'command': {
// ignore; we already know how to reply
break;
case 'reply':
}
case 'reply': {
replies.put(file.getData());
break;
case 'snapshot':
}
case 'snapshot': {
if (folders.length > 1 && folder !== folders.slice(-1)[0]) {
console.log(folder, step, 'ignoring remaining steps from', folder);
return;
} else {
try {
// eslint-disable-next-line no-await-in-loop
// This nested await is safe because "terminal-control-flow",
// given that we do not consider the timing of a `console.log`
// to be a practical hazard.
// eslint-disable-next-line @jessie.js/no-nested-await
await it.snapshot(file.getText());
} catch (err) {
console.warn(err, 'while taking snapshot:', err);
}
}
break;
default:
}
default: {
console.log(`bad kind: ${kind}`);
throw RangeError(`bad kind: ${kind}`);
}
}
// This nested await is safe because "top-level-of-for-await".
// We introduced it to make the unbalanced awaits in the above switch
// statement safe.
// eslint-disable-next-line @jessie.js/no-nested-await
await null;
done.push([folder, seq, kind]);
}
}
Expand All @@ -268,6 +313,11 @@ export async function replayXSnap(
const storedOpts = JSON.parse(rd.file(optionsFn).getText());
console.log(folder, optionsFn, 'already spawned; ignoring:', storedOpts);
}
// This nested await is safe because "top-level-of-for-await".
// It occurs at the top level of a for-await body, and so everything
// afterwards happens after a turn boundary, whether the awaited expression
// throws or not.
// eslint-disable-next-line @jessie.js/no-nested-await
await runSteps(rd, steps);
}

Expand Down
28 changes: 28 additions & 0 deletions packages/xsnap/src/xsnap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@ export function xsnap(options) {
*/
async function runToIdle() {
for (;;) {
// This nested await is safe because "terminal-throw-control-flow".
//
// If the awaited expression throws, no further code in this function
// runs. If it does not throw, then there is always a turn boundary
// before we proceed with each iteration.
// eslint-disable-next-line @jessie.js/no-nested-await
const iteration = await messagesFromXsnap.next(undefined);
if (iteration.done) {
xsnapProcess.kill();
Expand Down Expand Up @@ -204,7 +210,29 @@ export function xsnap(options) {
)}`,
);
} else if (message[0] === QUERY) {
// This nested await is safe because "not-my-problem".
//
// This nested await introduces no unsafety beyond that introduced
// by the next await, since it is earlier within the same control
// flow branch, and is top level within that branch.
// eslint-disable-next-line @jessie.js/no-nested-await
const commandResult = await handleCommand(message.subarray(1));
// This nested await is ALMOST safe because "terminal-control-flow".
// This one appears nested in an unbalanced conditional, so we need
// to consider three cases:
// * This branch is taken and the awaited expression throws.
// In that case, the function exits would executing and further
// code.
// * This branch is taken and does not throw. In that case,
// it causes a turn boundary before the next iteration.
// * Another branch of the conditional is taken, skipping
// this statement and proceeding syncronously to the next
// iteration. The await at the top of the loop ensures that the
// remainder of the loop only happens after a turn boundary.
// The only remaining unsafety is that the awaited expression of
// that initial await may therefore be reached synchronously or
// asynchronously.
// eslint-disable-next-line @jessie.js/no-nested-await
await messagesToXsnap.next([QUERY_RESPONSE_BUF, commandResult]);
} else {
// unrecognized responses also kill the process
Expand Down