src: don't overwrite non-writable vm globals #10227

bnoordhuis · 2016-12-11T16:38:51Z

@fhinkel Can you take a look? I couldn't figure out what is_contextual_store is needed for. Tests pass without it.

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

fhinkel · 2016-12-11T17:54:44Z

LGTM.

I was trying to mirror when V8 does not set the property. https://github.com/v8/v8/blob/master/src/objects.cc#L4812

I think we don't need is_contextual_store, because in the VM context we always work on the global object.

benjamingr · 2016-12-11T23:10:16Z

test/parallel/test-vm-context.js

+vm.runInContext('x = 0', ctx);                      // Does not throw but x...
+assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.
+assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
+              /Cannot assign to read only property 'x'/);


I'm not sure checking the error string is a great idea here as it might change.

True, but we can update the test when it does. I opted to be specific because it needs to throw this particular exception. Any other exception probably means something is broken.

Fair enough

Check that the property doesn't have the read-only flag set before overwriting it. Fixes: nodejs#10223 PR-URL: nodejs#10227 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

bnoordhuis · 2016-12-13T12:18:34Z

Landed in 524f693, thanks for reviewing.

Check that the property doesn't have the read-only flag set before overwriting it. Fixes: #10223 PR-URL: #10227 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2016-12-28T16:56:32Z

This appears to have broken console logging with jsdom

#10492

cpojer · 2016-12-29T01:40:55Z

See jestjs/jest#2457 (comment) for @thealphanerd's repro. This also applies to globals like setTimeout and other timer related features. The only workaround I can find so far is to set the fields in a non-strict module, which seems quite hacky. Can we at least make Object.defineProperty work for these.

This reverts commit 524f693. Fixes: nodejs#10806 Fixes: nodejs#10492 Ref: nodejs#10227

MylesBorins · 2017-01-22T23:20:40Z

due to the revert I am opting to label this as do not land.

This reverts commit 524f693. Fixes: #10806 Fixes: #10492 Ref: #10227 PR-URL: #10920 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This reverts commit 524f693. Fixes: nodejs#10806 Fixes: nodejs#10492 Ref: nodejs#10227 PR-URL: nodejs#10920 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

fhinkel · 2017-02-01T18:01:30Z

@bnoordhuis, I know this is long closed and reverted, but I figured out what is_contextual_store is. Thought I leave a comment. It describes whether this.foo = 42 or foo = 42 was called. The second is contextual and should fail in strict mode if foo is used without declaration.

This test would rely on it, if it weren't for the CopyProperties() hack. The setter callback intercepts this.z = 1, doesn't copy z over if there is no check for is_contextual_store, and instead CopyProperties() copies z to the sandbox. So the test passes with or without is_contextual_store.

Check that the property doesn't have the read-only flag set before overwriting it. This is Ben Noordhuis previous commit, but keeping is_contextual_store. is_contextual_store describes whether this.foo = 42 or foo = 42 was called. The second is contextual and will fail in strict mode if foo is used without declaration. Therefore only do an early return if it is a contextual store. In particular, don't do an early return for Object.defineProperty(this, ...). Fixes: nodejs#10223 Refs: nodejs#10227

Check that the property doesn't have the read-only flag set before overwriting it. This is Ben Noordhuis previous commit, but keeping is_contextual_store. is_contextual_store describes whether this.foo = 42 or foo = 42 was called. The second is contextual and will fail in strict mode if foo is used without declaration. Therefore only do an early return if it is a contextual store. In particular, don't do an early return for Object.defineProperty(this, ...). Fixes: #10223 Refs: #10227 PR-URL: #11109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Check that the property doesn't have the read-only flag set before overwriting it. This is Ben Noordhuis previous commit, but keeping is_contextual_store. is_contextual_store describes whether this.foo = 42 or foo = 42 was called. The second is contextual and will fail in strict mode if foo is used without declaration. Therefore only do an early return if it is a contextual store. In particular, don't do an early return for Object.defineProperty(this, ...). Fixes: nodejs#10223 Refs: nodejs#10227 PR-URL: nodejs#11109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

bnoordhuis requested a review from fhinkel December 11, 2016 16:38

nodejs-github-bot added dont-land-on-v7.x c++ Issues and PRs that require attention from people who are familiar with C++. labels Dec 11, 2016

bnoordhuis mentioned this pull request Dec 11, 2016

Object.defineProperty on the global object does not handle 'writable' properly in vm context #10223

Closed

mscdex added the vm Issues and PRs related to the vm subsystem. label Dec 11, 2016

Fishrock123 mentioned this pull request Dec 11, 2016

attempt-backport still does not work Edit: now working?! nodejs/github-bot#100

Closed

fhinkel approved these changes Dec 11, 2016

View reviewed changes

benjamingr approved these changes Dec 11, 2016

View reviewed changes

nodejs-github-bot added the lts-watch-v6.x label Dec 12, 2016

targos removed the dont-land-on-v7.x label Dec 12, 2016

bnoordhuis force-pushed the fix10223 branch from 70710d2 to 524f693 Compare December 13, 2016 12:18

bnoordhuis closed this Dec 13, 2016

bnoordhuis deleted the fix10223 branch December 13, 2016 12:18

nodejs-github-bot added the lts-watch-v6.x label Dec 13, 2016

bnoordhuis merged commit 524f693 into nodejs:master Dec 13, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

MylesBorins mentioned this pull request Dec 28, 2016

Regression in Jest with jsdom #10492

Closed

MylesBorins mentioned this pull request Dec 28, 2016

[WIP] Fix Console Logging in jsdom Env jestjs/jest#2457

Closed

addaleax added a commit to addaleax/node that referenced this pull request Jan 20, 2017

Revert "src: don't overwrite non-writable vm globals"

179ea5e

This reverts commit 524f693. Fixes: nodejs#10806 Fixes: nodejs#10492 Ref: nodejs#10227

MylesBorins added dont-land-on-v4.x and removed lts-watch-v6.x labels Jan 22, 2017

snyk-bot mentioned this pull request Apr 24, 2023

[Snyk] Security upgrade eslint from 4.19.1 to 5.0.0 aliscco/alisco-node#97

Open

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#109

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#122

Open

[Snyk] Fix for 17 vulnerabilities aliscco/alisco-node#127

Open

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#128

Open

This was referenced Apr 25, 2023

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#177

Open

[Snyk] Fix for 7 vulnerabilities aliscco/alisco-node#182

Open

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#183

Open

aliscco mentioned this pull request Apr 26, 2023

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#195

Open

snyk-bot mentioned this pull request Apr 26, 2023

[Snyk] Security upgrade eslint from 4.19.1 to 5.0.0 aliscco/alisco-node#249

Open

aliscco mentioned this pull request Apr 28, 2023

[Snyk] Fix for 10 vulnerabilities aliscco/alisco-node#309

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Security upgrade eslint from 4.19.1 to 5.0.0 aliscco/alisco-node#336

Open

aliscco mentioned this pull request Apr 29, 2023

[Snyk] Fix for 7 vulnerabilities aliscco/alisco-node#356

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: don't overwrite non-writable vm globals #10227

src: don't overwrite non-writable vm globals #10227

bnoordhuis commented Dec 11, 2016 •

edited

Loading

fhinkel commented Dec 11, 2016 •

edited

Loading

benjamingr Dec 11, 2016

bnoordhuis Dec 12, 2016

benjamingr Dec 12, 2016

bnoordhuis commented Dec 13, 2016

MylesBorins commented Dec 28, 2016

cpojer commented Dec 29, 2016

MylesBorins commented Jan 22, 2017

fhinkel commented Feb 1, 2017

src: don't overwrite non-writable vm globals #10227

src: don't overwrite non-writable vm globals #10227

Conversation

bnoordhuis commented Dec 11, 2016 • edited Loading

fhinkel commented Dec 11, 2016 • edited Loading

benjamingr Dec 11, 2016

Choose a reason for hiding this comment

bnoordhuis Dec 12, 2016

Choose a reason for hiding this comment

benjamingr Dec 12, 2016

Choose a reason for hiding this comment

bnoordhuis commented Dec 13, 2016

MylesBorins commented Dec 28, 2016

cpojer commented Dec 29, 2016

MylesBorins commented Jan 22, 2017

fhinkel commented Feb 1, 2017

bnoordhuis commented Dec 11, 2016 •

edited

Loading

fhinkel commented Dec 11, 2016 •

edited

Loading