[Streams] Verify stream usage within the same isolated world [chromium/src : main]
Adam Rice (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
3 comments
stream->DCheckWrapperWorld(script_state);Is this a sufficient fix for non-debug builds?
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
stream->DCheckWrapperWorld(script_state);Is this a sufficient fix for non-debug builds?
I've concluded that it isn't. We don't typically write tests involving the interaction of the same API in extensions and the main page. Problems are only likely to show up in release builds. I am going to redo this.
Related details
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +0 |
1 comment
stream->DCheckWrapperWorld(script_state);Adam RiceIs this a sufficient fix for non-debug builds?
I've concluded that it isn't. We don't typically write tests involving the interaction of the same API in extensions and the main page. Problems are only likely to show up in release builds. I am going to redo this.
Let me unset my attention set then while you're reworking the CL.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Commit-Queue | +1 |
2 comments
I'm not sure we need a blank line here.
This is actually in the style guide. From https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
```
In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test
the stuff in dir2/foo2.h, order your includes as follows:
1. dir2/foo2.h.
2. A blank line
3. C system headers, and any other headers in angle brackets with the .h
extension, e.g., <unistd.h>, <stdlib.h>, <Python.h>.
4. A blank line
5. C++ standard library headers (without file extension), e.g., <algorithm>
<cstddef>.
6. A blank line
7. Other libraries' .h files.
8. A blank line
9. Your project's .h files.
```
I actually never noticed rule 8 and haven't followed it. But rule 4 is generally followed in Chromium I think.
Adam RiceIs this a sufficient fix for non-debug builds?
Nidhi JajuI've concluded that it isn't. We don't typically write tests involving the interaction of the same API in extensions and the main page. Problems are only likely to show up in release builds. I am going to redo this.
Let me unset my attention set then while you're reworking the CL.
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
CHECK_EQ(wrapper_world_id_, script_state->World().GetWorldId());Is it fine to trigger a crash if this is violated since a malicious script could intentionally pass a stream across this boundary and call `.cancel()`? I'm wondering if we should instead throw an exception instead at least for the web-exposed API entry points?
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
CHECK_EQ(wrapper_world_id_, script_state->World().GetWorldId());Is it fine to trigger a crash if this is violated since a malicious script could intentionally pass a stream across this boundary and call `.cancel()`? I'm wondering if we should instead throw an exception instead at least for the web-exposed API entry points?
If there is a way to pass a stream object to a different isolated world that would already be a security bug by itself. Crashing in that case is the safe thing to do.
`postMessage` is not a problem, because in that case the stream in the destination realm is not actually the same C++ object.
Related details
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju voted Code-Review+1![]( "Open in Gerrit")
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |