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

v8: introduce v8.emitCodeGenFromStringEvent() #34863

Closed
wants to merge 2 commits into from
Closed

v8: introduce v8.emitCodeGenFromStringEvent() #34863

wants to merge 2 commits into from

Conversation

vdeturckheim
Copy link
Member

@vdeturckheim vdeturckheim commented Aug 21, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

As of today, it is not possible to intercept calls made to eval as the method can't really be monkeypatched without breaking its behavior.
Thanksfully there is an API in V8 that can be used to place a callback when code is generated from string (eval and new Function(string). This PR introduces a way to listen on theses events from userland.

If the --disallow-code-generation-from-strings flag is used, calling emitCodeGenFromStringEvent will have no effect as calls to eval and new Function will fail anyway.

After calling v8.emitCodeGenFromStringEvent(), each time a call is made to eval or new function, an codeGenerationFromString event will be emitted on process with the code as argument.

cc @fraxken @bmeck @cjihrig @targos :)

This introduces emitCodeGenFromStringEvent in
the v8 module to intercept calls made to eval
and Function constructor.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Aug 21, 2020
@devsnek
Copy link
Member

devsnek commented Aug 21, 2020

I will need to think about this more but my initial thought here is that this should be scoped to vm contexts.

@addaleax
Copy link
Member

Wouldn’t it be easier to do the same thing we do for e.g. the signal events, that is, use process.on('newListener') and process.on('removeListener') to check whether there are any listeners for this event, and if so, enable the feature?

@vdeturckheim
Copy link
Member Author

@devsnek that's a very fair point. I considered it too. AFAIK, vm is used to instanciate modules and enabling this could lead to potential perf issues on this path if the listeners are slow. Also, such behavior can be added to vm through monkeypatching so I don't have a real need for this there.
I settled with handling only the capabilities of the V8 API there but it might not be the more logical path.

@addaleax I explored a bit in this direction. The issue I met was that the flow of disabling it is pretty sensitive:
The state of --disallow-code-generation-from-strings should be respected so we might need ot add some kind of state somewhere to check if the value of IsCodeGenerationFromStringsAllowed() is set at process level or not.

Given that this is a bit niche, I went with this simpler API. But what you suggest is a good UX IMO so I'm open to move toward that (I will maybe need a hand to decide where to put this state I'm mentionning).

@devsnek
Copy link
Member

devsnek commented Aug 21, 2020

I'm really just not sure that this should exist at the scope of the main node context. Probably at worst it can leak implementation details between libraries.

@vdeturckheim
Copy link
Member Author

@devsnek do you think there could be another way to audit calls made to eval at runtime? This is actually the end goal of this PR for me.

@devsnek
Copy link
Member

devsnek commented Aug 21, 2020

@vdeturckheim a native addon perhaps?

@vdeturckheim
Copy link
Member Author

vdeturckheim commented Aug 21, 2020

@devsnek IMO it can be useful to many to have such in core - the user experience of native addons can be pretty complicated in some cases. Also,this makes a feature of V8 available in userland more than creating a new feature.

@vdeturckheim
Copy link
Member Author

@addaleax I think I have a solution actually. I will update soon with something that follows your suggestion 😄

@vdeturckheim
Copy link
Member Author

closing as it is a double of #35157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants