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.

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

Marshal fails to rehydrate errors in Chrome, Firefox, Safari #2198

Closed
kriskowal opened this issue Apr 3, 2024 · 10 comments · Fixed by #2200 or #2232
Closed

Marshal fails to rehydrate errors in Chrome, Firefox, Safari #2198

kriskowal opened this issue Apr 3, 2024 · 10 comments · Fixed by #2200 or #2232
Assignees
Labels
bug Something isn't working

Comments

@kriskowal
Copy link
Member

Describe the bug

Scene: a web page CapTP client connects over WebSocket to a remote CapTP endpoint. The client invokes a remote method. The server throws an error. The server logs that error. The client receives the error. However, instead of reporting an error with the same message as observed on the server, we see an error indicating that Marshal was unable to hydrate the error.

Chrome:

Error#2: Passable Error stack own property must be a data property: Object

Firefox:

Error#2: Passable Error has extra unpassed property fileName

Safari

Error#2:"Passable Error has extra unpassed property""line"

Steps to reproduce

To reproduce in the Endo repository:

In endo/packages/cli, create err.js

export const make = async powers => {
  await E(powers).bogus();
};

Then, also in endo/packages/cli:

bin/endo purge --force
bin/endo install err.js --name err --listen 3333 --powers NONE --open

This will open your system browser to a weblet. There will be one of the above errors in the JavaScript console.

Expected behavior

An error should be reported that has the same message as the error reported on the server.

For the isolated reproduction above, you can find the server’s error in one of the Endo worker logs. I apologize ahead of time that this is not yet polished: bin/endo log will show you the most recent daemon logs and the error should be in there. bin/endo where log will tell you where that log file is so you can open it elsewhere. bin/endo where state will show you where all the logs are.

Platform environment

Any system, any browser.

Additional context

Screenshots

@kriskowal kriskowal added the bug Something isn't working label Apr 3, 2024
@kriskowal kriskowal changed the title Marshal fails to rehydrate errors in Chrome and Firefox Marshal fails to rehydrate errors in Chrome, Firefox, Safari Apr 3, 2024
@erights
Copy link
Contributor

erights commented Apr 19, 2024

Reopening because the previous fix #2200 did not update regular non-browser tests so they would work on a platform with v8's problematic stack-accessor behavior. I accidentally discovered this when testing locally with Node 21, which our CI does not yet test.

@erights
Copy link
Contributor

erights commented Apr 19, 2024

Root cause:

At https://chromium-review.googlesource.com/c/v8/v8/+/4459251 v8 changed their error stack property into an own accessor property with per-instance getters and setters.

@erights
Copy link
Contributor

erights commented Apr 21, 2024

@kriskowal , @frazarshad , since this is in the same ballpark, I went ahead and co-assigned this to the three of us.

erights added a commit that referenced this issue Apr 22, 2024
closes: #2198 
refs: #2230 #2200
https://chromium-review.googlesource.com/c/v8/v8/+/4459251 #2229 #2231


## Description

Alternative to #2229 that just hacks `harden` to directly repair a
problematic error own stack accessor property, replacing it with a data
property.

### Security Considerations

Both before and after this PR, `passStyleOf` will reject errors with the
v8 problematic error own stack accessor property, preventing the
unsafety at stake here. However, this would mean that much existing code
that used to be correct will break when run on a v8 with this problem.

### Scaling Considerations

Avoids any extra overhead on platforms without this problem, including
all platforms other than v8.

### Documentation Considerations

probably none. This PR essentially avoids the need to document the v8
problem that it masks.

### Testing Considerations

Only needed to repair one test to use `harden` rather than `freeze`, in
a case where `harden` was more natural anyway.

### Compatibility Considerations

This PR enables more errors to pass that check without further changes
to user code. #2229 had similar goals, but would still require more
changes to user code than this PR. This is demonstrated by all the test
code in #2229 that needed to be fixed that does not need to be fixed in
this PR.

### Upgrade Considerations

none

- ~[ ] Includes `*BREAKING*:` in the commit message with migration
instructions for any breaking change.~
- ~[ ] Updates `NEWS.md` for user-facing changes.~
@erights
Copy link
Contributor

erights commented Apr 24, 2024

https://issues.chromium.org/issues/40279506 is now public, so we can cite it.

@erights
Copy link
Contributor

erights commented Apr 24, 2024

First reported at tc39/proposal-error-stacks#26 (comment)

@michaelficarra
Copy link

@erights Since the own accessor used the same two powerful functions for get/set, couldn't you deny access to them through virtualising all built-ins that read the property descriptor and Reflect.get/Reflect.set? In the shim, you can just check if their identity matches the values of the get/set fields that would be returned.

@mhofman
Copy link
Contributor

mhofman commented Apr 25, 2024

@erights Since the own accessor used the same two powerful functions for get/set, couldn't you deny access to them through virtualising all built-ins that read the property descriptor and Reflect.get/Reflect.set? In the shim, you can just check if their identity matches the values of the get/set fields that would be returned.

We tried and implemented that at some point, but it's really expensive!

@michaelficarra
Copy link

Given the choice between safe and fast...

@mhofman
Copy link
Contributor

mhofman commented Apr 25, 2024

It's also really hard to get right. In our first implementation last year, I had found 2 flaws that would still have leaked the stack accessors. It also layered terribly. We have first run code that captures all original intrinsics for internal use. We do mark some of them as dangerous to make sure we can audit their usage. To avoid having to re-audit all the internal uses of these reflection methods, we had to "patch" them before being captured. All that for a single engine's misbehavior. Ultimately, we decided to tolerate the capability leak these accessors enable (proxies enable similar leaks), and instead fix whatever error objects we encounter with the accessors.

@erights erights mentioned this issue May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment