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

process: introduce codeGenerationFromString event #48295

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

watson
Copy link
Member

@watson watson commented Jun 2, 2023

In this PR I'm picking up the work from the now closed #35157. I've updated the code from #35157 to now work with the newest version of main.

The 'codeGenerationFromString' event is emitted on process when a call is made to eval or Function.

The goal is to provide a way to listen to unsafe code generation from strings as it is not possible to monkeypatch eval.

The event will only be emitted if there is at least one listener on it and removing all listeners on this event will result in the handler in V8 to never be called. In other words, if there is no listener on this event, there should be no performance impact on calling eval or the Function constructor.

While working on the to-do's below, I'll add WIP commits to the PR which modifies the V8 code directly. These commits are ment only as a place to discuss the V8 changes that needs to land upstream, and before marking this PR as ready for review, these commits obviously needs to be removed.

Todo

  • Figure out which code changes are needed in V8 to ensure that the callback provided to SetModifyCodeGenerationFromStringsCallback can safely call JavaScript code. Today, this is problematic, as an exception thrown inside of the JavaScript code might be swallowed. This is because the calling V8 C++ function doesn't expect the callback to call into JavaScript.
  • Land the above mentioned V8 patch upstream in the V8 project.
  • Hopefully be able to backport this patch to the version of V8 that Node.js is currently using (and back to older versions of V8 used by older versions of Node.js if we want this new codeGenerationFromString event to be backported to older versions of Node.js.

Tests

Below are a few example scripts that can be used to verify if uncaught exceptions from within the event callback are swallowed or not.

❌ This script should generate an uncaught exception, but doesn't:

process.on('codeGenerationFromString', (code) => {
  console.log('Hello from callback!')
  throw new Error('boom!!!') // error is silenced
})

eval('1+1')

✅ This script should generate an uncaught exception, and it does:

process.on('codeGenerationFromString', (code) => {
  console.log('Hello from callback!')
  throw new Error('boom!!!') // error is not silenced
})

console.log('result of eval:', eval('1+1'))

✅ This script should generate an uncaught exception, and it does:

process.on('codeGenerationFromString', (code) => {
  console.log('Hello from callback!')
  throw new Error('boom!!!') // error is not silenced
})

eval('1+1')
queueMicrotask(() => {})

❌ This script should generate an uncaught exception, but doesn't:

process.on('codeGenerationFromString', (code) => {
  console.log('Hello from callback!')
  throw new Error('boom!!!') // error is silenced
})

eval('1+1')

process.removeAllListeners('codeGenerationFromString')
queueMicrotask(() => {})

vdeturckheim and others added 4 commits June 1, 2023 17:07
The 'codeGenerationFromString' event is emitted
when a call is made to `eval` or `new Function`.

Co-authored-by: Thomas Watson <[email protected]>
With newer versions of v8 this doesn't seem to be needed
I know this needs to be done upstream.
But while experimenting, it's easiest to do inline.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Jun 2, 2023
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Code changes themselves LGTM but I'd be interested to see the performance impact.

Comment on lines +499 to +500
bool val = args[0]->BooleanValue(args.GetIsolate());
if (val) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool val = args[0]->BooleanValue(args.GetIsolate());
if (val) {
if (args[0]->IsTrue()) {

bool val = args[0]->BooleanValue(args.GetIsolate());
if (val) {
context->AllowCodeGenerationFromStrings(false);
isolate->SetModifyCodeGenerationFromStringsCallback(CodeGenCallback);
Copy link
Member

Choose a reason for hiding this comment

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

This would replace the ModifyCodeGenerationFromStrings callback we already have for source maps and also override the isolate setting from embedders. I think we could simply configure a per-environment flag here to decide whether we want to call ProcessEmit in the existing ModifyCodeGenerationFromStrings callback.

I also noticed that AllowCodeGenerationFromStrings() is never set to true in our contexts because we always delegate that decision to the callback, so the fast path is never taken. Not sure if that's intentional though, it seems to me that it should only be set to false when we need the callback (e.g. when source maps are enabled)? @legendecas

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the callback can be installed when needed (either with the cli flag --enable-source-maps or programmatically process.setSourceMapsEnabled(true)). However, we'll need an additional flag to memorize if an embedder's callback is installed in SetIsolateMiscHandlers instead so that the embedder's callback is not overridden.

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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants