From b3fcd1a79969b2d7e7d1e50ecaccdc6e60e6ee1d Mon Sep 17 00:00:00 2001 From: Roman Ilin Date: Sat, 7 Sep 2024 15:46:16 +0300 Subject: [PATCH 1/3] lib: fix workers thread-safety issues --- lib/protocol/crypto/src/binding.cc | 72 ++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/lib/protocol/crypto/src/binding.cc b/lib/protocol/crypto/src/binding.cc index 559d7682..574cbb75 100644 --- a/lib/protocol/crypto/src/binding.cc +++ b/lib/protocol/crypto/src/binding.cc @@ -84,11 +84,15 @@ class ChaChaPolyCipher : public ObjectWrap { SetPrototypeMethod(tpl, "encrypt", Encrypt); SetPrototypeMethod(tpl, "free", Free); - constructor().Reset(Nan::GetFunction(tpl).ToLocalChecked()); + Local func = Nan::GetFunction(tpl).ToLocalChecked(); + Local context = Nan::GetCurrentContext(); + v8::Isolate* isolate = context->GetIsolate(); + + constructor().Set(isolate, func); Nan::Set(target, Nan::New("ChaChaPolyCipher").ToLocalChecked(), - Nan::GetFunction(tpl).ToLocalChecked()); + func); } private: @@ -387,8 +391,8 @@ class ChaChaPolyCipher : public ObjectWrap { obj->clear(); } - static inline Nan::Persistent & constructor() { - static Nan::Persistent my_constructor; + static inline v8::Eternal & constructor() { + static v8::Eternal my_constructor; return my_constructor; } @@ -414,11 +418,15 @@ class AESGCMCipher : public ObjectWrap { SetPrototypeMethod(tpl, "encrypt", Encrypt); SetPrototypeMethod(tpl, "free", Free); - constructor().Reset(Nan::GetFunction(tpl).ToLocalChecked()); + Local func = Nan::GetFunction(tpl).ToLocalChecked(); + Local context = Nan::GetCurrentContext(); + v8::Isolate* isolate = context->GetIsolate(); + + constructor().Set(isolate, func); Nan::Set(target, Nan::New("AESGCMCipher").ToLocalChecked(), - Nan::GetFunction(tpl).ToLocalChecked()); + func); } private: @@ -633,8 +641,8 @@ class AESGCMCipher : public ObjectWrap { obj->clear(); } - static inline Nan::Persistent & constructor() { - static Nan::Persistent my_constructor; + static inline v8::Eternal & constructor() { + static v8::Eternal my_constructor; return my_constructor; } @@ -651,11 +659,15 @@ class GenericCipher : public ObjectWrap { SetPrototypeMethod(tpl, "encrypt", Encrypt); SetPrototypeMethod(tpl, "free", Free); - constructor().Reset(Nan::GetFunction(tpl).ToLocalChecked()); + Local func = Nan::GetFunction(tpl).ToLocalChecked(); + Local context = Nan::GetCurrentContext(); + v8::Isolate* isolate = context->GetIsolate(); + + constructor().Set(isolate, func); Nan::Set(target, Nan::New("GenericCipher").ToLocalChecked(), - Nan::GetFunction(tpl).ToLocalChecked()); + func); } private: @@ -1014,8 +1026,8 @@ class GenericCipher : public ObjectWrap { obj->clear(); } - static inline Nan::Persistent & constructor() { - static Nan::Persistent my_constructor; + static inline v8::Eternal & constructor() { + static v8::Eternal my_constructor; return my_constructor; } @@ -1044,11 +1056,15 @@ class ChaChaPolyDecipher : public ObjectWrap { SetPrototypeMethod(tpl, "decryptLen", DecryptLen); SetPrototypeMethod(tpl, "free", Free); - constructor().Reset(Nan::GetFunction(tpl).ToLocalChecked()); + Local func = Nan::GetFunction(tpl).ToLocalChecked(); + Local context = Nan::GetCurrentContext(); + v8::Isolate* isolate = context->GetIsolate(); + + constructor().Set(isolate, func); Nan::Set(target, Nan::New("ChaChaPolyDecipher").ToLocalChecked(), - Nan::GetFunction(tpl).ToLocalChecked()); + func); } private: @@ -1440,8 +1456,8 @@ class ChaChaPolyDecipher : public ObjectWrap { obj->clear(); } - static inline Nan::Persistent & constructor() { - static Nan::Persistent my_constructor; + static inline v8::Eternal & constructor() { + static v8::Eternal my_constructor; return my_constructor; } @@ -1468,11 +1484,15 @@ class AESGCMDecipher : public ObjectWrap { SetPrototypeMethod(tpl, "decrypt", Decrypt); SetPrototypeMethod(tpl, "free", Free); - constructor().Reset(Nan::GetFunction(tpl).ToLocalChecked()); + Local func = Nan::GetFunction(tpl).ToLocalChecked(); + Local context = Nan::GetCurrentContext(); + v8::Isolate* isolate = context->GetIsolate(); + + constructor().Set(isolate, func); Nan::Set(target, Nan::New("AESGCMDecipher").ToLocalChecked(), - Nan::GetFunction(tpl).ToLocalChecked()); + func); } private: @@ -1697,8 +1717,8 @@ class AESGCMDecipher : public ObjectWrap { obj->clear(); } - static inline Nan::Persistent & constructor() { - static Nan::Persistent my_constructor; + static inline v8::Eternal & constructor() { + static v8::Eternal my_constructor; return my_constructor; } @@ -1716,11 +1736,15 @@ class GenericDecipher : public ObjectWrap { SetPrototypeMethod(tpl, "decrypt", Decrypt); SetPrototypeMethod(tpl, "free", Free); - constructor().Reset(Nan::GetFunction(tpl).ToLocalChecked()); + Local func = Nan::GetFunction(tpl).ToLocalChecked(); + Local context = Nan::GetCurrentContext(); + v8::Isolate* isolate = context->GetIsolate(); + + constructor().Set(isolate, func); Nan::Set(target, Nan::New("GenericDecipher").ToLocalChecked(), - Nan::GetFunction(tpl).ToLocalChecked()); + func); } private: @@ -2183,8 +2207,8 @@ class GenericDecipher : public ObjectWrap { obj->clear(); } - static inline Nan::Persistent & constructor() { - static Nan::Persistent my_constructor; + static inline v8::Eternal & constructor() { + static v8::Eternal my_constructor; return my_constructor; } From bcd8064df0b41f97dd31adf06f3d134a3e50fa77 Mon Sep 17 00:00:00 2001 From: Roman Ilin Date: Sun, 2 Feb 2025 05:02:39 +0300 Subject: [PATCH 2/3] test: add test for workers thread-safety issues fix --- test/test-worker-imports.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 test/test-worker-imports.js diff --git a/test/test-worker-imports.js b/test/test-worker-imports.js new file mode 100644 index 00000000..75adbf3b --- /dev/null +++ b/test/test-worker-imports.js @@ -0,0 +1,19 @@ +// Test for thread-safety issues caused by subsequent imports of the module +// in worker threads: https://github.com/mscdex/ssh2/issues/1393. +// Each subsequent worker increases probability of abnormal termination. +// The probability of a false pass due to zero response becomes negligible +// for 4 consecutive workers. +'use strict'; + +const { Worker, isMainThread } = require('worker_threads'); +require('../lib/index.js'); + +if (isMainThread) { + async function runWorker() { + return new Promise((r) => new Worker(__filename).on("exit", r)); + } + runWorker() + .then(runWorker) + .then(runWorker) + .then(runWorker); +} From 243e193b829fd4617a9c91ac73e854d6dd6c45ce Mon Sep 17 00:00:00 2001 From: Roman Ilin Date: Wed, 5 Feb 2025 20:32:46 +0300 Subject: [PATCH 3/3] test: bypass workers test for early Node.js versions --- test/test-worker-imports.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/test-worker-imports.js b/test/test-worker-imports.js index 75adbf3b..8152d9ec 100644 --- a/test/test-worker-imports.js +++ b/test/test-worker-imports.js @@ -5,12 +5,18 @@ // for 4 consecutive workers. 'use strict'; -const { Worker, isMainThread } = require('worker_threads'); +let Worker, isMainThread; +try { + ({ Worker, isMainThread } = require('worker_threads')); +} catch (e) { + if (e.code !== 'MODULE_NOT_FOUND') throw e; + process.exit(0); +} require('../lib/index.js'); if (isMainThread) { async function runWorker() { - return new Promise((r) => new Worker(__filename).on("exit", r)); + return new Promise((r) => new Worker(__filename).on('exit', r)); } runWorker() .then(runWorker)