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

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

Closed
TimothyGu opened this issue Dec 6, 2017 · 4 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@TimothyGu
Copy link
Member

  • 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)
@apapirovski apapirovski added v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem. labels Dec 6, 2017
@fhinkel
Copy link
Member

fhinkel commented Dec 21, 2017

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

@apapirovski
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

TimothyGu added a commit to TimothyGu/node that referenced this issue Aug 18, 2018
This allows using a Proxy object as the sandbox for a VM context.

Fixes: nodejs#17480
Fixes: nodejs#17481
@TimothyGu
Copy link
Member Author

TimothyGu commented Aug 24, 2018

Landed in fa543c0...85c356c.

Ugh. Fixed in 85c356c / #22390.

TimothyGu added a commit that referenced this issue Aug 24, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54492}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@9eb96bb
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
TimothyGu pushed a commit that referenced this issue Aug 24, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55142}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
TimothyGu added a commit that referenced this issue Aug 24, 2018
This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Aug 24, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54492}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@9eb96bb
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Aug 24, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55142}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Aug 24, 2018
This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54492}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@9eb96bb
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55142}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Sep 7, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55142}

PR-URL: nodejs#22390
Fixes: nodejs#17480
Fixes: nodejs#17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Sep 7, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55142}

PR-URL: nodejs#22390
Fixes: nodejs#17480
Fixes: nodejs#17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this issue Sep 18, 2018
This is a re-land of a commit landed as part of
nodejs#22390.

---

This allows using a Proxy object as the sandbox for a VM context.

Refs: nodejs#22390
Fixes: nodejs#17480
Fixes: nodejs#17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
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
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants