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

vm: properly handle defining props on any value #46615

Merged
merged 4 commits into from
Mar 18, 2023

Conversation

dubzzz
Copy link
Contributor

@dubzzz dubzzz commented Feb 11, 2023

While it was supposed to fix most of the remaining issues, #46458 missed some in strict mode.

This PR adds some additional checks.

It also clarifies what we are really checking to execute or not the GetReturnValue. The is_function I resurrected from 17.x was actually not a good option as it made some assignations of non-function values starting to fail. It seems that while it was relevant in 17.x, it was probably not the best option for 18.x.

Fix #43129


Here is a quick overview of what worked or not for each version of node:

OwnPropertySymbols should list new symbols
const totoSymbol = Symbol.for('toto');
Object.defineProperty(global, totoSymbol, {
  enumerable: true,
  writable: true,
  value: 4,
  configurable: true,
});
assert.strictEqual(global[totoSymbol], 4);
assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));
OwnPropertyNames should list new keys
const totoKey = 'toto';
Object.defineProperty(global, totoKey, {
  enumerable: true,
  writable: true,
  value: 5,
  configurable: true,
});
assert.strictEqual(global[totoKey], 5);
assert.ok(Object.getOwnPropertyNames(global).includes(totoKey));
Override proxy with function
window.globalProxy.prop1 = () => {};
assert.strictEqual(window.globalProxy.prop1, 42);
Override proxy with number
window.globalProxy.prop2 = 143;
assert.strictEqual(window.globalProxy.prop2, 43);
Override proxy with number on basic value
window.globalProxy.prop3 = 144;
assert.strictEqual(window.globalProxy.prop3, 144);
Override proxy with number on unknown value
window.globalProxy.prop4 = 145;
assert.strictEqual(window.globalProxy.prop4, 145);
Override proxy with function on symbol
window.globalProxy[sym1] = () => {};
assert.strictEqual(window.globalProxy[sym1], 46);
Override proxy with number
window.globalProxy[sym2] = 147;
assert.strictEqual(window.globalProxy[sym2], 47);
Override proxy with number on basic value on symbol
window.globalProxy[sym3] = 148;
assert.strictEqual(window.globalProxy[sym3], 148);
Override proxy with number on unknown value on symbol
window.globalProxy[sym4] = 149;
assert.strictEqual(window.globalProxy[sym4], 149);

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Feb 11, 2023
@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 11, 2023

I ran JS tests but forgot to check the linting. Will run linters and update the PR.

src/node_contextify.cc Outdated Show resolved Hide resolved
src/node_contextify.cc Outdated Show resolved Hide resolved
@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 17, 2023

@aduh95 @targos Any opinion on this change? As you approved my previous attempt, you might be interested into this version. This time it seems to fix all remaining issues, including the tickets that were not resolved yet.

Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

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

lgtm

@vdeturckheim vdeturckheim added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@vdeturckheim
Copy link
Member

@addaleax would ou have time to take another look? rom what I understand it looks good but you have more context (pun intended) than me here

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@vdeturckheim Not really, unfortunately … I’d mostly just look at the test changes here, and they seem reasonable to me, so I have no concerns about this landing in general

if (is_declared_on_sandbox &&
ctx->sandbox()
->GetOwnPropertyDescriptor(context, property)
.ToLocal(&desc) &&
Copy link
Member

Choose a reason for hiding this comment

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

We probably should be returning early here if this .ToLocal() returns false (i.e. getting the property descriptor throws an exception), and probably also if the HasOwnProperty calls below return empty Maybes, rather than still trying to call sandbox()->Set() later on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of that too but I finally concluded: calling the set will have the benefit to apply the right behaviour. But no strong opinion at all 🙂

Copy link
Member

Choose a reason for hiding this comment

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

But should we really do the Set() call if there if an exception occurred? Right now, this code is doing the equivalent of

let err;
let is_get_set_property = false;
let desc;
try {
  desc = Object.getOwnPropertyDescriptor(sandbox, property);
} catch (err_) {
  err = err_;
}
if (err === undefined && typeof desc === 'object' && desc !== null) {
  let has_get = false, has_set = false;
  try {
    has_get = Object.hasOwn(desc, 'get');
  } catch (err_) {
    err = err_;
  }
  try {
    has_set = Object.hasOwn(desc, 'set');
  } catch (err_) {
    err = err_;
  }
  is_get_set_property = has_get || has_set;
}
sandbox[property] = value;
if (err) throw err;
if (is_get_set_property) 
  return value;

when it might be a bit more intuitive to do the equivalent of

let is_get_set_property = false;
const desc = Object.getOwnPropertyDescriptor(sandbox, property);
if (typeof desc === 'object' && desc !== null) {
  is_get_set_property = Object.hasOwn(desc, 'get') || Object.hasOwn(desc, 'set');
}
sandbox[property] = value;
if (is_get_set_property) 
  return value;

.

If we do want to unconditionally set sandbox[property] = value, then maybe we should do that before the rest of the checks (and also return early in case of an exception during that assignment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember well I mostly preserved the call to Set as it was done all the time starting at 18.2.0. So I may have thought 'well let's not drop it from these cases for now to reduce risks of breaking things possibly not well covered'. But in the meantime as I covered the change by many extra tests (e2e), we may give it a try. I'll not be able to do it in the coming days, but I can either attempt to do on this PR when I'll be back or on another one. Anyway, I'll add some more checks if I do add it so that we get sure that these possible edge cases also get well covered (in some scenarios I tried during my manual checks, calling or not the Set was similar).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do want to unconditionally set sandbox[property] = value, then maybe we should do that before the rest of the checks (and also return early in case of an exception during that assignment)?

I will update the PR to follow your suggestion by moving the line:

USE(ctx->sandbox()->Set(context, property, value));

Before the if statement.

It might be a bit more intuitive to do the equivalent of

Will try to rework it asap too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the suggestions @addaleax

I reworked a bit the code since your last review. I'm just wondering if you had better/shorter suggestions for the two following if blocks?

  Local<Value> desc;
  if (is_declared_on_sandbox &&
      ctx->sandbox()
          ->GetOwnPropertyDescriptor(context, property)
          .ToLocal(&desc) &&
      desc->IsObject()) {
    if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) ||
        desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false))

Copy link
Member

Choose a reason for hiding this comment

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

The first one seems correct as-is (and generally, it’s a bit unfortunate that “correct” and “shorter” are usually conflicting goals when working with V8 Maybes).

I think what still needs to be addressed here is:

and also return early in case of an exception during that assignment

i.e. replace USE(ctx->sandbox()->Set(context, property, value)); with if (ctx->sandbox()->Set(context, property, value).IsNothing()) return; or similar

and currently, the PR here uses desc_obj->HasOwnProperty(context, env->get_string()).FromJust(), which should probably also use proper error handling (i.e. .To() or .FromMaybe() instead of .FromJust()) since otherwise an exception here would crash the process.

While it was supposed to fix most of the remaining issues,
nodejs#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.
@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 9, 2023

Cc @addaleax @vdeturckheim, sorry for the force push, I rebased the branch on main and stashed commits related to the feedbacks you gave me altogether. The last suggestions made by @addaleax have been handled in the commit called Apply comments of 2nd review.

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2023
@nodejs-github-bot
Copy link
Collaborator

@targos targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Mar 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit aa6e9c8 into nodejs:main Mar 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in aa6e9c8

targos pushed a commit that referenced this pull request Mar 18, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Mar 18, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
dubzzz added a commit to dubzzz/node that referenced this pull request Apr 15, 2023
This PR is a follow-up of nodejs#46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19.
dubzzz added a commit to dubzzz/node that referenced this pull request Apr 15, 2023
This PR is a follow-up of nodejs#46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.
nodejs-github-bot pushed a commit that referenced this pull request May 26, 2023
This PR is a follow-up of #46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.

PR-URL: #47572
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2023
This PR is a follow-up of #46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.

PR-URL: #47572
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau added the lts-watch-v18.x PRs that may need to be released in v18.x. label Jun 8, 2023
MylesBorins pushed a commit that referenced this pull request Jun 23, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

I landed this on v18.x-staging in fb90b6b

@aduh95 aduh95 added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed lts-watch-v18.x PRs that may need to be released in v18.x. labels Jun 23, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
This PR is a follow-up of #46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.

PR-URL: #47572
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
This PR is a follow-up of nodejs#46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.

PR-URL: nodejs#47572
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This PR is a follow-up of nodejs#46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.

PR-URL: nodejs#47572
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This PR is a follow-up of nodejs#46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.

PR-URL: nodejs#47572
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vm.runInContext() breaks delete on v18.2.0
8 participants