Skip to content

Object.getOwnPropertyDescriptor operator in VM uses [[Get]] instead of [[GetOwnProperty]] #17481

Description

@TimothyGu
  • Version: master
  • Platform: all
  • Subsystem: vm

Very similar to #17480, except the issue now is with Object.getOwnPropertyDescriptor rather than in operator.

const globals = {};
const handlers = {};
const realHandlers = Reflect.ownKeys(Reflect).reduce((handlers, p) => {
  handlers[p] = (t, ...args) => {
    // Avoid printing the Receiver argument, which can lead to an infinite loop.
    console.log(p, ...(p === 'get' || p === 'set' ? args.slice(0, -1) : args));
    return Reflect[p](t, ...args);
  };
  return handlers;
}, {});
const proxy = vm.createContext(new Proxy(globals, handlers));

// Indirection needed to mitigate against #17465
// https://github.com/nodejs/node/issues/17465
const globalProxy = vm.runInContext('this', proxy);
for (const k of Reflect.ownKeys(globalProxy)) {
  Object.defineProperty(globals, k, Object.getOwnPropertyDescriptor(globalProxy, k));
}
Object.assign(handlers, realHandlers);

Object.getOwnPropertyDescriptor(proxy, "a")
  // prints "getOwnPropertyDescriptor a"
  // returns undefined

vm.runInContext('Object.getOwnPropertyDescriptor(this, "a")', proxy);
  // prints "get Object"
  // prints "getOwnPropertyDescriptor a"
  // prints "get a"
  // prints "get a"
  // returns { value: undefined,
  //           writable: true,
  //           enumerable: false,
  //           configurable: true } (because of https://github.com/nodejs/node/issues/17465)

Activity

  1. added
    v8 engineIssues and PRs related to the V8 dependency.
    vmIssues and PRs related to the vm subsystem.
    on Dec 6, 2017
  2. fhinkel commented on Dec 21, 2017

    @fhinkel
    Contributor

    See https://chromium.googlesource.com/v8/v8/+/d5fbf7c5c3f8f9b46b75f674771f3533c7e3e24d. Let's give it some canary coverage, then we can backport it. Thanks @TimothyGu

  3. apapirovski commented on Apr 12, 2018

    @apapirovski
    Contributor

    @fhinkel @TimothyGu do you happen to have a status update on this? Still an issue? Has the fix landed in Node.js since then?

  4. bnoordhuis commented on Apr 12, 2018

    @bnoordhuis
    Member

    The upstream commit was reverted again in https://chromium-review.googlesource.com/c/v8/v8/+/850355 because of performance issues.

  5. TimothyGu commented on Aug 24, 2018

    @TimothyGu
    MemberAuthor

    Landed in fa543c0...85c356c.

    Ugh. Fixed in 85c356c / #22390.

  6. added a commit that references this issue on Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    v8 engineIssues and PRs related to the V8 dependency.vmIssues and PRs related to the vm subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions