-
Notifications
You must be signed in to change notification settings - Fork 72
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.
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
v8 error taming should leverage prepareStackTrace
#1798
Labels
enhancement
New feature or request
Comments
kriskowal
added
the
kriskowal-review-2024-01
Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024
label
Jan 6, 2024
aj-agoric
removed
the
kriskowal-review-2024-01
Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024
label
Jan 31, 2024
This was referenced Feb 28, 2024
Hi @mhofman , I've co-assigned myself to this. |
I believe I explored this, and that didn't work |
1 task
erights
added a commit
that referenced
this issue
Jul 16, 2024
Closes: #2348 Refs: Agoric/agoric-sdk#9711 #1798 #1799 Agoric/agoric-sdk#8662 Agoric/agoric-sdk#9700 ## Description Prior to this PR, when you ran on Ava on Node a test written in TypeScript, you'd see something like the following in your stack traces. ``` boot/test/bootstrapTests/stack-linenumbers.test.ts:1:104 ``` This is because the TypeScript compiler compiles a TypeScripy file into one line of JavaScript with a sourceMap that should map back into original source positions. Node specifically makes use of that sourceMap to produce original line-numbers. However, Node does this in a way that resists virtualization, so the normal SES error taming cannot use this sourceMap info. By default, this PR does not change this behavior. However it recognizes a new `SUPPRESS_NODE_ERROR_TAMING` environment variable. With the `SUPPRESS_NODE_ERROR_TAMING` environment variable absent or set to `'disabled'`, you should still see stack traces as shown above However, if you also set the `SUPPRESS_NODE_ERROR_TAMING` environment variable `'enabled'`, for example by doing ```sh $ export SUPPRESS_NODE_ERROR_TAMING=enabled ``` at a bash shell, then when you run this test you should instead see something like ``` boot/test/bootstrapTests/stack-linenumbers.test.ts:40:32 ``` At Agoric/agoric-sdk#9711 I both - turn this PR into an agoric-sdk patch of endo, in order to emulate this fix until the next endo-release-agoric-sdk-sync cycle, and - add a test case that emits an error stack trace from an Ava test case written in TypeScript, to test that it works. ### Security Considerations This new behavior only applies when `errorTaming: 'unsafe'`, on v8, and with this new environment variable enabled. Setting `errorTaming: 'unsafe'` already flags to sacrifice some security for a better debugging experience. But the loss of security is moderate enough --- mostly confidentiality rather than integrity --- that some may chose this setting for some production purposes. The new behavior is a more severe loss of security that really should be used ***only during development***, not production, when even a severe loss of security is usually not an issue. ### Scaling Considerations none ### Documentation Considerations The behavior prior to this PR or without this environment variable enabled is an unpleasant debugging experience. However, developers won't know how to repair it, or even that it can be repaired, without explanation. Even then, the difficultly of discovery in a problem. The names `SUPPRESS_NODE_ERROR_TAMING` and the settings `'enabled'` and `'disabled'` are by no means clear expressions of what this does. Reviewers, ***better names would be appreciated!*** ### Testing Considerations The point. As developers write and run tests written in TypeScript, they need to iterate with problems revealed by the tests, for which they need good line numbers, including into the test code. When the environment variable is enabled, the new behavior broke some SES tests written specifically to test the old behavior. This would not happen under CI because the environment variable is not set by default, and so may not have been noticed. But it was revealed in local testing. To repair this, this PR also sets those tests up to set `process.env.SUPPRESS_NODE_ERROR_TAMING` to `'disabled'` before lockdown, protecting those tests from the external environment variable setting. Awkwardly, at the moment Agoric/agoric-sdk#9711 serves as the only test of this PR. This is because I failed to figure out how to configure things so I can run TypeScript tests under Ava, like Agoric/agoric-sdk#9711 does. I tried cargo culting the configs that seemed relevant, but it didn't work. Reviewers, if you let me know how to do this, I'll duplicate the test case from Agoric/agoric-sdk#9711 here, which would be good. ### Compatibility Considerations With the environment variable absent or disabled, there should be zero difference in behavior, so none. In a development environment where this environment variable is enabled, some stack traces will be different. But outside of SES itself, nothing should depend on the contents of stack traces, so again none. ### Upgrade Considerations No upgrade considerations. Nothing BREAKING. - [x] Update `NEWS.md` for user-facing changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is the Problem Being Solved?
Error.prepareStackTrace
can be used by Node.js to apply source maps to the captured stack trace. However SES sets its ownprepareStackTrace
during v8 Error taming. Instead of completely discarding it. Maybe instead it should leverage as much as possible the existingprepareStackTrace
to prepare its own stack strings?Furthermore, if the start compartment set a
prepareStackTrace
after lockdown, it seems the returned value will be used for the.stack
property instead of the empty string, even iferrorTaming
is set to'safe'
:endo/packages/ses/src/error/tame-v8-error-constructor.js
Lines 301 to 304 in 2a85f8a
Description of the Design
Use the original
prepareStackTrace
to generate the stack string.Alternatively implement source map support in endo itself (maybe once part of the error handling logic has been ejected from SES as a trusted shim?, see #945). While node's
prepareStackTrace
implementation is not very modular, it should be possible to rebuild it on top offindSourceMap
.Security Considerations
None particularly. The start compartment is already in position to decide how lockdown should tame errors, and can also set
prepareStackTrace
after lockdownScaling Considerations
None
Test Plan
load some minified source which throws errors in Node.js with
--use-source-maps
and verify stack traces are mapped.Upgrade Considerations
None particular
The text was updated successfully, but these errors were encountered: