fix: reject promises in the arguments to syscall.callNow() #1766
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We use the same bootstrap vat files multiple times in this test file, so this amortizes the bundling cost up front. We were previously doing this for the kernel bundles, but this adds the vat bundles too. I also changed the tests to run serially (using `test.serial()` instead of `test()`), because we aren't speeding anything up by thrashing around among a dozen simultaneous tests. JS remains single-threaded (and threads remain evil).
Vats which hold device nodes (`d-NN` references) can use `syscall.callNow()` on them, to make a synchronous invocation (which can return data). The outbound arguments and return data are capdata, which is translated through c-lists just like regular `syscall.send()` and promise resolution. However devices do not (currently) handle Promises at all. The kernel-to-device c-list translation will panic the kernel if asked to translate a promise reference (`kpNN`). Vats should not be able to panic the kernel, even if we give them access to a device node. This changes the vat-to-kernel translator to reject promise references in the arguments of `callNow`, making it a vat-fatal error. This will terminate the vat, but leave the kernel running. In the long run (#1346), devices should accept Promises, but it will take more work (and probably require devices to operate on a much lower level than vats do). But this should fix the immediate kernel panic. Note that killing a vat is not exactly friendly either. The bug described in issue #1358 was triggered by user REPL input causing the HTTP vat to try sending a Promise into the Command device, killing the kernel. With this change, this will instead kill the HTTP vat, which breaks the REPL, rendering the system mostly unusable. But at least the attribution is correct. We have another fix in the works that will change liveslots.js to catch this situation during the call to `D(devnode).methname(args)`, which should reduce the blast radius to merely throw an exception in `D()`, rather than killing the whole vat. refs #1358
warner
force-pushed
the
1358-cleanup-device-tests
branch
from
3a45930 to
2e4c10c
Compare
warner
force-pushed
the
1358-callnow-promises
branch
from
4d5b059 to
3ca1700
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Vats which hold device nodes (
d-NNreferences) can usesyscall.callNow()on them, to make a synchronous invocation (which can return data). The
outbound arguments and return data are capdata, which is translated through
c-lists just like regular
syscall.send()and promise resolution.However devices do not (currently) handle Promises at all. The
kernel-to-device c-list translation will panic the kernel if asked to
translate a promise reference (
kpNN).Vats should not be able to panic the kernel, even if we give them access to a
device node. This changes the vat-to-kernel translator to reject promise
references in the arguments of
callNow, making it a vat-fatal error. Thiswill terminate the vat, but leave the kernel running.
In the long run (#1346), devices should accept Promises, but it will take
more work (and probably require devices to operate on a much lower level than
vats do). But this should fix the immediate kernel panic.
Note that killing a vat is not exactly friendly either. The bug described in
issue #1358 was triggered by user REPL input causing the HTTP vat to try
sending a Promise into the Command device, killing the kernel. With this
change, this will instead kill the HTTP vat, which breaks the REPL, rendering
the system mostly unusable. But at least the attribution is correct.
We have another fix in the works that will change liveslots.js to catch this
situation during the call to
D(devnode).methname(args), which should reducethe blast radius to merely throw an exception in
D(), rather than killingthe whole vat.
refs #1358