Skip to content

src: make accessors immune to context confusion #1238

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Mar 23, 2015

Conversation

bnoordhuis
Copy link
Member

It's possible for an accessor or named interceptor to get called with
a different execution context than the one it lives in, see the test
case for an example using the debug API.

This commit fortifies against that by passing the environment as a
data property instead of looking it up through the current context.

Fixes: #1190 (again)

R=@indutny? /cc @ofrobots

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/362/

var Debug = vm.runInDebugContext('Debug');
var breaks = 0;

function ondebugevent(evt, exc) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: add comment explaining that exceptions from debug event listeners are swallowed.

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 23, 2015
inline Environment* Environment::GetCurrent(
const v8::PropertyCallbackInfo<v8::Value>& info) {
const v8::PropertyCallbackInfo<T>& info) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this is actually needed anywhere. Is it just a new feature?

@indutny
Copy link
Member

indutny commented Mar 23, 2015

LGTM

It's possible for an accessor or named interceptor to get called with
a different execution context than the one it lives in, see the test
case for an example using the debug API.

This commit fortifies against that by passing the environment as a
data property instead of looking it up through the current context.

Fixes: nodejs#1190 (again)
PR-URL: nodejs#1238
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Remove some unnecessary environment lookups and delete a few superfluous
HandleScope variables.

PR-URL: nodejs#1238
Reviewed-By: Fedor Indutny <fedor@indutny.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants