repl,worker: fix crash when SharedArrayBuffer disabled by codebytere · Pull Request #39718 · nodejs/node · GitHub

Closes #39717.

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)

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. 
        
           let cachedCwd = ''; 
        
           let lastCounter = -1; 
        
           const originalCwd = process.cwd; 
        
           process.cwd = function() { 
        
             const currentCounter = Atomics.load(cwdCounter, 0); 
        
             if (currentCounter === lastCounter) 
        
               return cachedCwd; 
        
             lastCounter = currentCounter; 
        
             cachedCwd = originalCwd(); 
        
             return cachedCwd; 
        
           }; 
        
           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).

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

Is it repl or worker (the subsystem)?

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

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

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

@codebytere repl,worker: fix crash ... would work :)

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

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

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

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

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

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

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

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.

LGTM with my comments addressed.

Suggested change

      
            const { isMainThread, Worker } = require('worker_threads');
          
            // Regression test for https://github.com/nodejs/node/issues/39717.
          
            const w = new Worker(__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;
          
              const w = new Worker(__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?

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

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.

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

Comment thread benchmark/worker/atomics-wait.js Outdated

Trott reviewed Aug 10, 2021

View reviewed changes

Comment thread lib/internal/worker.js Outdated

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 disabled Aug 10, 2021

codebytere force-pushed the fix-sab-crash branch from 56f1396 to 51830bb Compare 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

Comment thread test/parallel/test-repl-no-sab.js Outdated

codebytere force-pushed the fix-sab-crash branch 2 times, most recently from 6e29e56 to 489fa03 Compare 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 7c5a3e0 Compare 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 324adc2 Compare 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 9ff558a Compare 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.
	let cachedCwd = '';
	let lastCounter = -1;
	const originalCwd = process.cwd;

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

                   process.cwd = function() {
+                    // SharedArrayBuffers can be disabled with --no-harmony-sharedarraybuffer.
+                    if (typeof SharedArrayBuffer === 'undefined') return originalCwd();

               function main({ n }) {
+                if (typeof SharedArrayBuffer === 'undefined') {
+                  throw new Error('SharedArrayBuffers must be enabled to run this benchmark');
+                }

repl,worker: fix crash when SharedArrayBuffer disabled by codebytere · Pull Request #39718 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl,worker: fix crash when SharedArrayBuffer disabled - #39718

repl,worker: fix crash when SharedArrayBuffer disabled#39718codebytere wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom codebytere:fix-sab-crashcodebytere/node:fix-sab-crashCopy head branch name to clipboard

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!

BridgeAR Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

Trott Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 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

repl,worker: fix crash when SharedArrayBuffer disabled#39718
codebytere wants to merge 2 commits into
nodejs:mainfrom
codebytere:fix-sab-crash

codebytere commented Aug 9, 2021 •
edited
Loading

targos commented Aug 9, 2021 •
edited
Loading

Trott commented Sep 2, 2021 •
edited
Loading