repl,worker: fix crash when SharedArrayBuffer disabled#39718

codebytere · 2021-08-09T16:39:41Z

It's possible for SharedArrayBuffers to be disabled with --no-harmony-sharedarraybuffer so we first need to check that this isn't the case before attempting to use them in the repl or the following crash occurs:

electron_node on git:a3d0cc7244 ❯ node --no-harmony-sharedarraybuffer 6:25PM Welcome to Node.js v16.2.0. Type ".help" for more information. > snode:internal/readline/emitKeypressEvents:71 throw err; ^ TypeError: SharedArrayBuffer is not a constructor at node:internal/worker:96:32 at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:312:7) at nativeModuleRequire (node:internal/bootstrap/loaders:341:14) at node:worker_threads:11:5 at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:312:7) at nativeModuleRequire (node:internal/bootstrap/loaders:341:14) at node:inspector:32:26 at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:312:7) at nativeModuleRequire (node:internal/bootstrap/loaders:341:14) at sendInspectorCommand (node:internal/util/inspector:14:21)

BridgeAR

This will likely break process.cwd() after changing the directory as it will return a cached value.
cwd() should also be protected in that case:

node/lib/internal/main/worker_thread.js

Lines 138 to 152 in 82b1b55

// The counter is only passed to the workers created by the main thread, not
// to workers created by other workers.
letcachedCwd='';
letlastCounter=-1;
constoriginalCwd=process.cwd;
process.cwd=function(){
constcurrentCounter=Atomics.load(cwdCounter,0);
if(currentCounter===lastCounter)
returncachedCwd;
lastCounter=currentCounter;
cachedCwd=originalCwd();
returncachedCwd;
};
workerIo.sharedCwdCounter=cwdCounter;

I am fine doing that, I just wonder if this is a common situation at all. AFAIK we mostly do not check all possible configurations and do not officially support them?

If we want to support the flag, we should also add a test (while such test can't really protect from other potential SharedArrayBuffer usages).

codebytere · 2021-08-09T17:18:17Z

@BridgeAR happy to add a test - the other reason i'm interested in this in particular is because it's disabled in the web (see here) unless cross origin isolation is enabled, so this crashes the renderer process in some cases in Electron as well.

targos · 2021-08-09T17:54:32Z

Is it repl or worker (the subsystem)?

codebytere · 2021-08-09T23:30:40Z

@targos i guess sort of both? I can make it whatever you think would be preferable, but it'd be hit anytime anyone invoked repl.start() or when dealing with cwd in workers.

benchmark/worker/atomics-wait.js

lib/internal/worker.js

Trott

LGTM with lint fix and either a test or a comment (or both).

targos · 2021-08-10T06:23:21Z

@targos i guess sort of both? I can make it whatever you think would be preferable, but it'd be hit anytime anyone invoked repl.start() or when dealing with cwd in workers.

@codebytererepl,worker: fix crash ... would work :)

codebytere · 2021-08-10T08:16:48Z

@BridgeAR i think that should do it - let me know if you had something else in mind tho!

BridgeAR

LGTM. Just left a comment how the test could be improved.

test/parallel/test-repl-no-sab.js

nodejs-github-bot · 2021-08-10T09:34:23Z

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

Trott · 2021-08-11T14:18:37Z

There's a relevant failure in CI. The test added here fails when invoked with test/tools.py --worker.

codebytere · 2021-09-02T09:48:03Z

@Trott are you potentially able to replicate locally?

i'm seeing

node on git:fix-sab-crash ❯ tools/test.py --worker test/parallel/test-worker-no-sab === release test-worker-no-sab === Path: parallel/test-worker-no-sab Command: out/Release/node --no-harmony-sharedarraybuffer /Users/codebytere/Developer/node/tools/run-worker.js /Users/codebytere/Developer/node/test/parallel/test-worker-no-sab.js [00:00|% 100|+ 0|- 1]: Done

passing when i run locally 🤔

nodejs-github-bot · 2021-09-02T09:50:34Z

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

Trott · 2021-09-02T15:07:09Z

@Trott are you potentially able to replicate locally?

i'm seeing

node on git:fix-sab-crash ❯ tools/test.py --worker test/parallel/test-worker-no-sab === release test-worker-no-sab === Path: parallel/test-worker-no-sab Command: out/Release/node --no-harmony-sharedarraybuffer /Users/codebytere/Developer/node/tools/run-worker.js /Users/codebytere/Developer/node/test/parallel/test-worker-no-sab.js [00:00|% 100|+ 0|- 1]: Done

passing when i run locally 🤔

@codebytere That's not passing. The 1 would be in the + cell if it was passing, and the 0 would b e in the - cell. (You can see the difference if you run it again without --worker. Or you can try running it with --worker but run ttest/parallel/test-worker-no-s* to see one test pass and one test fail.)

The lack of output, though, is odd....

Trott · 2021-09-02T15:09:50Z

The lack of output, though, is odd....

The lack of output is legit. (Maybe we should update tools/run-worker.js to provide output in this situation?)

To replicate without the Python script, run this out/Release/node --no-harmony-sharedarraybuffer /Users/trott/io.js/tools/run-worker.js /Users/trott/io.js/test/parallel/test-worker-no-sab.js. It will return no output, but exit with an error code.

BridgeAR

LGTM with my comments addressed.

BridgeAR · 2021-12-04T01:10:38Z

test/parallel/test-worker-no-sab.js

+const{ isMainThread, Worker }=require('worker_threads');
+
+// Regression test for https://github.com/nodejs/node/issues/39717.
+
+constw=newWorker(__filename);
+
+w.on('exit',common.mustCall((status)=>{
+assert.strictEqual(status,2);
+}));
+
+if(!isMainThread)process.exit(2);


Suggested change
const{ isMainThread, Worker }=require('worker_threads');
// Regression test for https://github.com/nodejs/node/issues/39717.
constw=newWorker(__filename);
w.on('exit',common.mustCall((status)=>{
assert.strictEqual(status,2);
}));
if(!isMainThread)process.exit(2);
const{ Worker }=require('worker_threads');
// Regression test for https://github.com/nodejs/node/issues/39717.
// Do not use isMainThread so that this test itself can be run inside a Worker.
if(!process.env.HAS_STARTED_WORKER){
process.env.HAS_STARTED_WORKER=1;
constw=newWorker(__filename);
w.on('exit',common.mustCall((status)=>{
assert.strictEqual(status,2);
}));
}else{
process.exit(2);
}

@codebytere Are you ok if we commit this suggestion, run tests, check with @BridgeAR if they can then approve the PR,, and hopefully land? Or is there something about this suggestion that would make you reluctant to do that?

BridgeAR · 2021-12-04T01:11:24Z

lib/internal/main/worker_thread.js


 process.cwd=function(){
+// SharedArrayBuffers can be disabled with --no-harmony-sharedarraybuffer.
+if(typeofSharedArrayBuffer==='undefined')returnoriginalCwd();


This is fine but we could prevent replacing process.cwd in the first place, so that the extra check is not needed when calling process.cwd().

BridgeAR · 2021-12-04T01:12:41Z

benchmark/worker/atomics-wait.js

 functionmain({ n }){
+if(typeofSharedArrayBuffer==='undefined'){
+thrownewError('SharedArrayBuffers must be enabled to run this benchmark');
+}


main is going to be executed more than once. As such, it's probably best to move the check to the top of the file.

codebytere · 2022-09-13T22:31:53Z

Closing as superseded by #41023

codebytere requested a review from BridgeAR August 9, 2021 16:39

nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Aug 9, 2021

codebytere added the repl Issues and PRs related to the REPL subsystem. label Aug 9, 2021

jasnell approved these changes Aug 9, 2021
View reviewed changes

BridgeAR reviewed Aug 9, 2021
View reviewed changes

Trott reviewed Aug 10, 2021
View reviewed changes

benchmark/worker/atomics-wait.js Outdated Show resolvedHide resolved

Trott reviewed Aug 10, 2021
View reviewed changes

lib/internal/worker.jsShow resolvedHide resolved

Trott approved these changes Aug 10, 2021
View reviewed changes

codebytere changed the title ~~repl: fix crash when SharedArrayBuffer disabled~~repl,worker: fix crash when SharedArrayBuffer disabledAug 10, 2021

codebytere force-pushed the fix-sab-crash branch from 56f1396 to 51830bbCompare August 10, 2021 08:16

codebytere requested a review from BridgeAR August 10, 2021 08:16

BridgeAR approved these changes Aug 10, 2021
View reviewed changes

test/parallel/test-repl-no-sab.js Outdated Show resolvedHide resolved

codebytere force-pushed the fix-sab-crash branch 2 times, most recently from 6e29e56 to 489fa03Compare August 10, 2021 09:12

addaleax approved these changes Aug 10, 2021
View reviewed changes

codebytere force-pushed the fix-sab-crash branch from 489fa03 to 7c5a3e0Compare August 10, 2021 09:25

codebytere removed the needs-ci PRs that need a full CI run. label Aug 10, 2021

codebytere force-pushed the fix-sab-crash branch from 7c5a3e0 to 324adc2Compare August 10, 2021 09:33

nodejs deleted a comment from nodejs-github-bot Aug 10, 2021

zcbenz approved these changes Aug 11, 2021
View reviewed changes

codebytere added 2 commits September 2, 2021 11:13

repl: fix crash when SharedArrayBuffer disabled
7c3ed50

test: add a SharedArrayBuffer worker test
9ff558a

codebytere force-pushed the fix-sab-crash branch from 324adc2 to 9ff558aCompare September 2, 2021 09:48

BridgeAR mentioned this pull request Nov 30, 2021
lib: do not crash using workers with disabled shared array buffers #41023
Merged

BridgeAR requested changes Dec 4, 2021
View reviewed changes

codebytere closed this Sep 13, 2022

codebytere deleted the fix-sab-crash branch September 13, 2022 22:31

	// The counter is only passed to the workers created by the main thread, not
	// to workers created by other workers.
	letcachedCwd='';
	letlastCounter=-1;
	constoriginalCwd=process.cwd;

	process.cwd=function(){
	constcurrentCounter=Atomics.load(cwdCounter,0);
	if(currentCounter===lastCounter)
	returncachedCwd;
	lastCounter=currentCounter;
	cachedCwd=originalCwd();
	returncachedCwd;
	};
	workerIo.sharedCwdCounter=cwdCounter;

Uh oh!

repl,worker: fix crash when SharedArrayBuffer disabled#39718

repl,worker: fix crash when SharedArrayBuffer disabled #39718

Uh oh!

Conversation

codebytere commented Aug 9, 2021

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere commented Aug 9, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Aug 9, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codebytere commented Aug 9, 2021

Uh oh!

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Aug 10, 2021

Uh oh!

codebytere commented Aug 10, 2021

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 10, 2021

Uh oh!

Trott commented Aug 11, 2021

Uh oh!

codebytere commented Sep 2, 2021

Uh oh!

nodejs-github-bot commented Sep 2, 2021

Uh oh!

Trott commented Sep 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Sep 2, 2021

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeARDec 4, 2021

Choose a reason for hiding this comment

Uh oh!

TrottMar 17, 2022

Choose a reason for hiding this comment

Uh oh!

BridgeARDec 4, 2021

Choose a reason for hiding this comment

Uh oh!

BridgeARDec 4, 2021

Choose a reason for hiding this comment

Uh oh!

codebytere commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codebytere commented Aug 9, 2021•
edited
Loading

targos commented Aug 9, 2021•
edited
Loading

Trott commented Sep 2, 2021•
edited
Loading