vm: properly handle defining props on any value

[dubzzz]

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));

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

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));

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with function

window.globalProxy.prop1 = () => {};
assert.strictEqual(window.globalProxy.prop1, 42);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with number

window.globalProxy.prop2 = 143;
assert.strictEqual(window.globalProxy.prop2, 43);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with number on basic value

window.globalProxy.prop3 = 144;
assert.strictEqual(window.globalProxy.prop3, 144);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with number on unknown value

window.globalProxy.prop4 = 145;
assert.strictEqual(window.globalProxy.prop4, 145);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with function on symbol

window.globalProxy[sym1] = () => {};
assert.strictEqual(window.globalProxy[sym1], 46);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with number

window.globalProxy[sym2] = 147;
assert.strictEqual(window.globalProxy[sym2], 47);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with number on basic value on symbol

window.globalProxy[sym3] = 148;
assert.strictEqual(window.globalProxy[sym3], 148);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

Override proxy with number on unknown value on symbol

window.globalProxy[sym4] = 149;
assert.strictEqual(window.globalProxy[sym4], 149);

• Pass on 17.9.0
• Pass on 18.0.0
• Pass on 18.2.0 (after src: always signal V8 for intercepted properties #42963)
• Pass after vm: properly support symbols on globals #46458

[dubzzz]

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

[dubzzz]

@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.

[vdeturckheim]

lgtm

[vdeturckheim]

@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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49974/

[addaleax]

@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

[addaleax]

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

[dubzzz]

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 🙂

[addaleax]

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)?

[dubzzz]

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).

[dubzzz]

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.

[dubzzz]

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))

[addaleax]

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.

[dubzzz]

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.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50289/

[nodejs-github-bot]

Landed in aa6e9c8

[MylesBorins]

I landed this on v18.x-staging in fb90b6b