From 135f144cf07e2769ba4f96f3ad8c00e4f9112144 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 20 Mar 2022 15:03:25 +0530 Subject: [PATCH 1/8] src,inspector: fix empty MaybeLocal crash Return early when the Inspector StringView to V8 String conversion fails and returns an empty MaybeLocal instead of running the invalid ToLocalChecked() assertion. Fixes: https://github.com/nodejs/node/issues/42407 Signed-off-by: Darshan Sen --- src/inspector_js_api.cc | 8 ++++---- .../parallel/test-repl-empty-maybelocal-crash.js | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-repl-empty-maybelocal-crash.js diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 8de1f8e7b0a88d..30a2e31361f47d 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -75,10 +75,10 @@ class JSBindingsConnection : public AsyncWrap { Isolate* isolate = env_->isolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(env_->context()); - MaybeLocal v8string = - String::NewFromTwoByte(isolate, message.characters16(), - NewStringType::kNormal, message.length()); - Local argument = v8string.ToLocalChecked().As(); + Local argument; + if (!String::NewFromTwoByte(isolate, message.characters16(), + NewStringType::kNormal, + message.length()).ToLocal(&argument)) return; connection_->OnMessage(argument); } diff --git a/test/parallel/test-repl-empty-maybelocal-crash.js b/test/parallel/test-repl-empty-maybelocal-crash.js new file mode 100644 index 00000000000000..3e2f3f42d98f11 --- /dev/null +++ b/test/parallel/test-repl-empty-maybelocal-crash.js @@ -0,0 +1,16 @@ +'use strict'; +require('../common'); + +// The process should not crash when the REPL receives the string, 'ss'. +// Test for https://github.com/nodejs/node/issues/42407. + +const repl = require('repl'); + +const r = repl.start(); + +r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); +r.write('var ss = buf.toString("binary");\n'); +r.write('ss'); +r.write('.'); + +r.close(); From a363b6e02dd8f30334234000f11b95f4b9c42ced Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 27 Mar 2022 12:28:34 +0530 Subject: [PATCH 2/8] test: set a timeout value Signed-off-by: Darshan Sen --- .../test-repl-empty-maybelocal-crash.js | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-repl-empty-maybelocal-crash.js b/test/parallel/test-repl-empty-maybelocal-crash.js index 3e2f3f42d98f11..281df99a348471 100644 --- a/test/parallel/test-repl-empty-maybelocal-crash.js +++ b/test/parallel/test-repl-empty-maybelocal-crash.js @@ -1,16 +1,23 @@ 'use strict'; -require('../common'); +const common = require('../common'); // The process should not crash when the REPL receives the string, 'ss'. // Test for https://github.com/nodejs/node/issues/42407. -const repl = require('repl'); +if (process.argv[2] === 'child') { + const repl = require('repl'); -const r = repl.start(); + const r = repl.start(); -r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); -r.write('var ss = buf.toString("binary");\n'); -r.write('ss'); -r.write('.'); + r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); + r.write('var ss = buf.toString("binary");\n'); + r.write('ss'); + r.write('.'); -r.close(); + r.close(); + return; +} + +require('child_process') + .fork(__filename, ['child'], { timeout: common.platformTimeout(15000) }) + .on('exit', (exitCode) => process.exit(exitCode === null ? 1 : exitCode)); From 364c9ceccecd43f509cf9d7ab48bf71c1e0c3722 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 27 Mar 2022 16:44:23 +0530 Subject: [PATCH 3/8] Revert "test: set a timeout value" This reverts commit a363b6e02dd8f30334234000f11b95f4b9c42ced. --- .../test-repl-empty-maybelocal-crash.js | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-repl-empty-maybelocal-crash.js b/test/parallel/test-repl-empty-maybelocal-crash.js index 281df99a348471..3e2f3f42d98f11 100644 --- a/test/parallel/test-repl-empty-maybelocal-crash.js +++ b/test/parallel/test-repl-empty-maybelocal-crash.js @@ -1,23 +1,16 @@ 'use strict'; -const common = require('../common'); +require('../common'); // The process should not crash when the REPL receives the string, 'ss'. // Test for https://github.com/nodejs/node/issues/42407. -if (process.argv[2] === 'child') { - const repl = require('repl'); +const repl = require('repl'); - const r = repl.start(); +const r = repl.start(); - r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); - r.write('var ss = buf.toString("binary");\n'); - r.write('ss'); - r.write('.'); +r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); +r.write('var ss = buf.toString("binary");\n'); +r.write('ss'); +r.write('.'); - r.close(); - return; -} - -require('child_process') - .fork(__filename, ['child'], { timeout: common.platformTimeout(15000) }) - .on('exit', (exitCode) => process.exit(exitCode === null ? 1 : exitCode)); +r.close(); From 0c4ab4e6e0dde10f65641901ba7484e1e8e398c7 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 27 Mar 2022 16:47:53 +0530 Subject: [PATCH 4/8] test: mark the test as SLOW in test/root.status Signed-off-by: Darshan Sen --- test/root.status | 1 + 1 file changed, 1 insertion(+) diff --git a/test/root.status b/test/root.status index 85d907b1f9cc07..fcfa2091e9d290 100644 --- a/test/root.status +++ b/test/root.status @@ -64,6 +64,7 @@ parallel/test-next-tick-fixed-queue-regression: SLOW parallel/test-npm-install: SLOW parallel/test-preload: SLOW parallel/test-repl: SLOW +parallel/test-repl-empty-maybelocal-crash: SLOW parallel/test-repl-history-navigation.js: SLOW parallel/test-repl-tab-complete: SLOW parallel/test-repl-top-level-await: SLOW From b461e2dd639a34530acbf3cb9bf585a7a41b1378 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 3 Apr 2022 17:45:06 +0530 Subject: [PATCH 5/8] test: set a timeout value Signed-off-by: Darshan Sen --- .../test-repl-empty-maybelocal-crash.js | 27 ++++++++++++++----- test/root.status | 1 - 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-repl-empty-maybelocal-crash.js b/test/parallel/test-repl-empty-maybelocal-crash.js index 3e2f3f42d98f11..c2c225175fe05d 100644 --- a/test/parallel/test-repl-empty-maybelocal-crash.js +++ b/test/parallel/test-repl-empty-maybelocal-crash.js @@ -4,13 +4,26 @@ require('../common'); // The process should not crash when the REPL receives the string, 'ss'. // Test for https://github.com/nodejs/node/issues/42407. -const repl = require('repl'); +if (process.argv[2] === 'child') { + const repl = require('repl'); -const r = repl.start(); + const r = repl.start(); -r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); -r.write('var ss = buf.toString("binary");\n'); -r.write('ss'); -r.write('.'); + r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); + r.write('var ss = buf.toString("binary");\n'); + r.write('ss'); + r.write('.'); -r.close(); + r.close(); + return; +} + +const controller = new AbortController(); +const { signal } = controller; +setTimeout(() => controller.abort(), 20_000); +require('child_process').fork(__filename, ['child'], { signal }) + .on('exit', (code) => process.exit(code === null ? 1 : code)) + .on('error', (err) => { + if (err.name === 'AbortError') process.exit(); + throw err; + }); diff --git a/test/root.status b/test/root.status index fcfa2091e9d290..85d907b1f9cc07 100644 --- a/test/root.status +++ b/test/root.status @@ -64,7 +64,6 @@ parallel/test-next-tick-fixed-queue-regression: SLOW parallel/test-npm-install: SLOW parallel/test-preload: SLOW parallel/test-repl: SLOW -parallel/test-repl-empty-maybelocal-crash: SLOW parallel/test-repl-history-navigation.js: SLOW parallel/test-repl-tab-complete: SLOW parallel/test-repl-top-level-await: SLOW From def11b0556fbb56596ab05bc9f0a0522b34bcd4f Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 3 Apr 2022 18:03:00 +0530 Subject: [PATCH 6/8] Revert "test: set a timeout value" This reverts commit b461e2dd639a34530acbf3cb9bf585a7a41b1378. --- .../test-repl-empty-maybelocal-crash.js | 27 +++++-------------- test/root.status | 1 + 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/test/parallel/test-repl-empty-maybelocal-crash.js b/test/parallel/test-repl-empty-maybelocal-crash.js index c2c225175fe05d..3e2f3f42d98f11 100644 --- a/test/parallel/test-repl-empty-maybelocal-crash.js +++ b/test/parallel/test-repl-empty-maybelocal-crash.js @@ -4,26 +4,13 @@ require('../common'); // The process should not crash when the REPL receives the string, 'ss'. // Test for https://github.com/nodejs/node/issues/42407. -if (process.argv[2] === 'child') { - const repl = require('repl'); +const repl = require('repl'); - const r = repl.start(); +const r = repl.start(); - r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); - r.write('var ss = buf.toString("binary");\n'); - r.write('ss'); - r.write('.'); +r.write('var buf = Buffer.from({length:200e6},(_,i) => i%256);\n'); +r.write('var ss = buf.toString("binary");\n'); +r.write('ss'); +r.write('.'); - r.close(); - return; -} - -const controller = new AbortController(); -const { signal } = controller; -setTimeout(() => controller.abort(), 20_000); -require('child_process').fork(__filename, ['child'], { signal }) - .on('exit', (code) => process.exit(code === null ? 1 : code)) - .on('error', (err) => { - if (err.name === 'AbortError') process.exit(); - throw err; - }); +r.close(); diff --git a/test/root.status b/test/root.status index 85d907b1f9cc07..fcfa2091e9d290 100644 --- a/test/root.status +++ b/test/root.status @@ -64,6 +64,7 @@ parallel/test-next-tick-fixed-queue-regression: SLOW parallel/test-npm-install: SLOW parallel/test-preload: SLOW parallel/test-repl: SLOW +parallel/test-repl-empty-maybelocal-crash: SLOW parallel/test-repl-history-navigation.js: SLOW parallel/test-repl-tab-complete: SLOW parallel/test-repl-top-level-await: SLOW From 9c4fb7bfba0a9d7f8e44a6d3cd88762eae6642f5 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 3 Apr 2022 18:09:05 +0530 Subject: [PATCH 7/8] test: move test to pummel Signed-off-by: Darshan Sen --- test/{parallel => pummel}/test-repl-empty-maybelocal-crash.js | 0 test/root.status | 1 - 2 files changed, 1 deletion(-) rename test/{parallel => pummel}/test-repl-empty-maybelocal-crash.js (100%) diff --git a/test/parallel/test-repl-empty-maybelocal-crash.js b/test/pummel/test-repl-empty-maybelocal-crash.js similarity index 100% rename from test/parallel/test-repl-empty-maybelocal-crash.js rename to test/pummel/test-repl-empty-maybelocal-crash.js diff --git a/test/root.status b/test/root.status index fcfa2091e9d290..85d907b1f9cc07 100644 --- a/test/root.status +++ b/test/root.status @@ -64,7 +64,6 @@ parallel/test-next-tick-fixed-queue-regression: SLOW parallel/test-npm-install: SLOW parallel/test-preload: SLOW parallel/test-repl: SLOW -parallel/test-repl-empty-maybelocal-crash: SLOW parallel/test-repl-history-navigation.js: SLOW parallel/test-repl-tab-complete: SLOW parallel/test-repl-top-level-await: SLOW From 358c4bf7e9bd915753b863c75d3eb9d38c33072a Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Fri, 8 Apr 2022 19:25:19 +0530 Subject: [PATCH 8/8] test: skip on armv7 bots Signed-off-by: Darshan Sen --- test/pummel/test-repl-empty-maybelocal-crash.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/pummel/test-repl-empty-maybelocal-crash.js b/test/pummel/test-repl-empty-maybelocal-crash.js index 3e2f3f42d98f11..84686e308c6157 100644 --- a/test/pummel/test-repl-empty-maybelocal-crash.js +++ b/test/pummel/test-repl-empty-maybelocal-crash.js @@ -1,5 +1,9 @@ 'use strict'; -require('../common'); +const common = require('../common'); + +if (process.config.variables.arm_version === '7') { + common.skip('Too slow for armv7 bots'); +} // The process should not crash when the REPL receives the string, 'ss'. // Test for https://github.com/nodejs/node/issues/42407.