From d04d07f6523faf459369783c199aa64bb19b7409 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:17:17 +0100 Subject: [PATCH 1/9] automated asan testing --- .github/workflows/test-dev.yml | 18 ++++++++++++++- README.md | 4 +++- include/nobuffer.h | 41 +++++++++++++++++++++++++++++++-- include/nofunction.h | 7 +++++- include/noobject.h | 7 +++++- test/binding.gyp | 17 +++++++++++++- test/fixtures/buffer.cc | 3 ++- test/framework.js | 27 ++++++++++++++++++---- test/napi-leaks-suppression.txt | 27 ++++++++++++++++++++++ test/single.js | 20 +++++++++++----- test/tests.test.js | 5 ++-- 11 files changed, 156 insertions(+), 20 deletions(-) create mode 100644 test/napi-leaks-suppression.txt diff --git a/.github/workflows/test-dev.yml b/.github/workflows/test-dev.yml index 03bf0b1..a91f6bb 100644 --- a/.github/workflows/test-dev.yml +++ b/.github/workflows/test-dev.yml @@ -7,7 +7,7 @@ on: branches: ["*"] jobs: - unit-tests: + release: runs-on: ${{ matrix.platform }} strategy: @@ -24,3 +24,19 @@ jobs: node-version: ${{ matrix.node-version }} - run: npm install - run: npm test + + + asan: + runs-on: ubuntu-22.04 + + steps: + - uses: actions/checkout@v3 + - name: Use Node.js 20.x + uses: actions/setup-node@v3 + with: + node-version: 20.x + - run: npm install + - run: npm test + env: + ENABLE_ASAN: 1 + LD_PRELOAD: /usr/lib/x86_64-linux-gnu/libasan.so.6.0.0 diff --git a/README.md b/README.md index 701d2b5..9ed0230 100644 --- a/README.md +++ b/README.md @@ -377,7 +377,9 @@ NOBIND_MODULE(buffer, m) { } ``` -Also, the default `Buffer` typemaps copy the data - which is not the most efficient way to transfer it. Sharing the memory between C++ and JavaScript is possible in many cases - but must be carefully implemented in a custom typemap. +When C++ returns a `Buffer` object, that buffer is considered owned and it will be freed upon the destruction of the Node.js `Buffer` object by the garbage-collector. + +When JavaScript passes a `Buffer` to a C++ method, data is copied - which is the safest but not the most efficient way to transfer it. Sharing the memory between C++ and JavaScript is possible in many cases - but must be carefully implemented in a custom typemap - especially when using async. ### Returning objects and factory functions diff --git a/include/nobuffer.h b/include/nobuffer.h index 7187a63..2c54853 100644 --- a/include/nobuffer.h +++ b/include/nobuffer.h @@ -8,11 +8,17 @@ namespace Nobind { namespace Typemap { // These typemaps transfer Buffers by copying the underlying data -// It is also possible to do it without copying if managing the GC +// It is also possible to do it without copying if properly managing the GC +// but this requires very through understanding of all the underlying +// mechanisms by the user of nobind17 // In C++ a Node::Buffer decomposes to std::pair using Buffer = std::pair; +// When calling C++ with a JS Buffer, a copy of the Buffer +// is created for the duration of the call +// This means that the typemap is completely safe for use with async +// code but it is not the most efficient way to transfer data template <> class FromJS { Buffer val_; @@ -27,15 +33,46 @@ template <> class FromJS { } inline Buffer Get() { return val_; } + + inline ~FromJS() { + delete[] val_.first; + val_.first = nullptr; + val_.second = 0; + } + + FromJS(const FromJS &) = delete; + + // When moving, we must be sure that the old copy + // won't get freed + inline FromJS(FromJS &&rhs) { + val_ = rhs.val_; + rhs.val_.first = nullptr; + rhs.val_.second = 0; + }; }; +// When receiving a Buffer from C++ we consider that +// ownership has been transferred to us template class ToJS { Napi::Env env_; Buffer val_; public: inline explicit ToJS(Napi::Env env, Buffer val) : env_(env), val_(val) {} - inline Napi::Value Get() { return Napi::Buffer::Copy(env_, val_.first, val_.second); } + inline Napi::Value Get() { +#ifdef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED + // Node-API does not support external buffers (Electron) + Napi::Buffer buffer = Napi::Buffer::Copy(env_, val_.first, val_.second); + delete[] val->data; + return buffer; +#else + // Node-API supports external buffers (Node.js) + return Napi::Buffer::New(env_, val_.first, val_.second, [](Napi::Env, uint8_t *data) { delete[] data; }); +#endif + } + + ToJS(const ToJS &) = delete; + ToJS(ToJS &&) = delete; }; } // namespace Typemap diff --git a/include/nofunction.h b/include/nofunction.h index 7c9a946..eb94172 100644 --- a/include/nofunction.h +++ b/include/nofunction.h @@ -96,7 +96,12 @@ inline Napi::Value FunctionWrapperAsync(const Napi::CallbackInfo &info, auto tasklet = new FunctionWrapperTasklet(env, deferred, {FromJSArgs(info, idx)...}); - CheckArgLength(env, idx, info.Length()); + try { + CheckArgLength(env, idx, info.Length()); + } catch (...) { + delete tasklet; + std::rethrow_exception(std::current_exception()); + } tasklet->Queue(); } catch (const std::exception &e) { diff --git a/include/noobject.h b/include/noobject.h index a1c2558..b9a16d8 100644 --- a/include/noobject.h +++ b/include/noobject.h @@ -237,7 +237,12 @@ template class NoObjectWrap : public Napi::ObjectWrap(env, deferred, self, {FromJSArgs(info, idx)...}); - CheckArgLength(env, idx, info.Length()); + try { + CheckArgLength(env, idx, info.Length()); + } catch (...) { + delete tasklet; + std::rethrow_exception(std::current_exception()); + } tasklet->Queue(); } catch (const std::exception &e) { diff --git a/test/binding.gyp b/test/binding.gyp index 32e9d37..8929a5d 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -1,4 +1,7 @@ { + 'variables': { + 'enable_asan%': 'false' + }, 'targets': [ { 'target_name': '<(test)', @@ -27,6 +30,18 @@ '-fvisibility=hidden', '-std=c++17' ] - } + }, + 'conditions': [ + ['enable_asan == "true"', { + 'cflags': [ + '-fsanitize=address' + ], + 'xcode_settings': { + 'OTHER_CPLUSPLUSFLAGS': [ + '-fsanitize=address' + ] + } + }] + ] } } diff --git a/test/fixtures/buffer.cc b/test/fixtures/buffer.cc index 1fbf6b5..c98136e 100644 --- a/test/fixtures/buffer.cc +++ b/test/fixtures/buffer.cc @@ -2,6 +2,7 @@ #include // Returns a buffer initialized to value +// Ownership is transferred to the caller void get_buffer(uint8_t *&buf, size_t &len, uint32_t value) { buf = new uint8_t[len]; auto *data = reinterpret_cast(buf); @@ -10,7 +11,7 @@ void get_buffer(uint8_t *&buf, size_t &len, uint32_t value) { } } -// Receives a buffer that must be initialized to value +// Receives a buffer and checks that is initialized to value void put_buffer(uint8_t *buf, size_t len, uint32_t value) { auto *data = reinterpret_cast(buf); for (size_t i = 0; i < len * sizeof(uint8_t) / sizeof(uint32_t); i++) { diff --git a/test/framework.js b/test/framework.js index d17a3e5..adfa8ca 100644 --- a/test/framework.js +++ b/test/framework.js @@ -4,6 +4,9 @@ const { execFileSync } = require('child_process'); const os = require('os'); const npx = os.platform() === 'win32' ? 'npx.cmd' : 'npx'; +const env = { ...process.env }; +// Do not pass asan when spawning processes +delete env.LD_PRELOAD; function list() { return fs.readdirSync(path.resolve(__dirname, 'tests')) @@ -11,6 +14,21 @@ function list() { .map((t) => t.split('.')[0]); } +function clean(test, stdio) { + try { + execFileSync(npx, [ + 'node-gyp', + 'clean', + `--test=${test}` + ], { stdio: stdio || 'pipe', cwd: __dirname, env }); + } catch (e) { + if (e.stdout) { + console.error(e.stdout); + } + throw e; + } +} + function configure(test, stdio, opts) { try { let include; @@ -26,7 +44,7 @@ function configure(test, stdio, opts) { ...(opts || []), `--test=${test}`, `--fixtures=${fixtures.map((f) => `fixtures/${f}.cc`).join(' ')}` - ], { stdio: stdio || 'pipe', cwd: __dirname }); + ], { stdio: stdio || 'pipe', cwd: __dirname, env }); } catch (e) { if (e.stdout) { console.error(e.stdout); @@ -39,11 +57,11 @@ function build(stdio) { execFileSync(npx, [ 'node-gyp', 'build' - ], { stdio: stdio || 'pipe', cwd: __dirname }); + ], { stdio: stdio || 'pipe', cwd: __dirname, env }); } -function load(test) { - globalThis.dll = require(path.resolve(__dirname, 'build', 'Release', `${test.split('.')[0]}.node`)); +function load(test, build) { + globalThis.dll = require(path.resolve(__dirname, 'build', build || 'Release', `${test.split('.')[0]}.node`)); } function register(test) { @@ -52,6 +70,7 @@ function register(test) { module.exports = { list, + clean, configure, build, load, diff --git a/test/napi-leaks-suppression.txt b/test/napi-leaks-suppression.txt new file mode 100644 index 0000000..bbc78e2 --- /dev/null +++ b/test/napi-leaks-suppression.txt @@ -0,0 +1,27 @@ +# All leaks listed here are non-repeat offenders +# (ie they happen only a fixed number of times per process execution) + +# Known leaks in Node-API +leak:napi_module_register +leak:napi_register_module_v1 + +# Known leaks in the Node.js runtime +leak:node::builtins::BuiltinLoader::LoadBuiltinSource +leak:ProcessWrap::OnExit +leak:StringBytes::Encode +leak:Realm::ExecuteBootstrapper + +# Known leaks in V8 +leak:Heap_GenerationalBarrierSlow +leak:Scavenger::ScavengePage +leak:Scavenger::Process +leak:CompactionSpace::Expand +leak:Heap::IterateRoots +leak:Heap::PerformGarbageCollection +leak:Heap::InsertIntoRememberedSetFromCode +leak:Heap::SetUpSpaces +leak:Heap::PerformGarbageCollection +leak:PagedSpace::RefillLabMain +leak:OldLargeObjectSpace::AllocateRaw +leak:BaselineBatchCompiler::EnqueueFunction +leak:Compiler::FinalizeTurbofanCompilationJob diff --git a/test/single.js b/test/single.js index b126c1f..ccf8932 100644 --- a/test/single.js +++ b/test/single.js @@ -1,6 +1,7 @@ const fs = require('fs'); const path = require('path'); const framework = require('./framework'); +const Mocha = require('mocha'); function usage() { console.log('Usage: node single.js '); @@ -22,20 +23,27 @@ if (!test) { } switch (process.argv[2]) { + case 'clean': + framework.clean(test, 'inherit'); + break; case 'configure': framework.configure(test, 'inherit', ['--debug']); break; + case 'configure-asan': + framework.configure(test, 'inherit', ['--debug', '--enable_asan']); + break; case 'build': framework.build('inherit'); break; case 'run': globalThis.dll = require(path.resolve(__dirname, 'build', 'Debug', `${test.split('.')[0]}.node`)); - globalThis.describe = (name, fn) => { - console.log(name); - fn(); - }; - globalThis.it = globalThis.describe; - framework.register(test); + const mocha = new Mocha({ ui: 'bdd' }); + mocha.addFile(path.resolve(__dirname, 'tests', test)); + mocha.run(function (failures) { + process.on('exit', function () { + process.exit(failures); + }); + }); break; default: usage(); diff --git a/test/tests.test.js b/test/tests.test.js index f257dbf..29c1e9f 100644 --- a/test/tests.test.js +++ b/test/tests.test.js @@ -5,9 +5,10 @@ describe('nobind', function () { for (const t of tests) { describe(t, () => { framework.register(t); - before('configure', () => framework.configure(t)); + before('clean', () => framework.clean(t)); + before('configure', () => framework.configure(t, undefined, process.env.ENABLE_ASAN && ['--debug', '--enable_asan'])); before('build', () => framework.build()); - before('load', () => framework.load(t)); + before('load', () => framework.load(t, process.env.ENABLE_ASAN && 'Debug')); after('GC', global.gc); }); } From 84593e86dfe95d555ddd830beb5e611461175a08 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:20:28 +0100 Subject: [PATCH 2/9] the asan suppression is tuned for Node 18 --- .github/workflows/test-dev.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test-dev.yml b/.github/workflows/test-dev.yml index a91f6bb..e3cf522 100644 --- a/.github/workflows/test-dev.yml +++ b/.github/workflows/test-dev.yml @@ -31,10 +31,10 @@ jobs: steps: - uses: actions/checkout@v3 - - name: Use Node.js 20.x + - name: Use Node.js 18.x uses: actions/setup-node@v3 with: - node-version: 20.x + node-version: 18.x - run: npm install - run: npm test env: From 2b48b3b37418523bdf4f757f032416f564450c88 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:26:10 +0100 Subject: [PATCH 3/9] add the suppressions --- .github/workflows/test-dev.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test-dev.yml b/.github/workflows/test-dev.yml index e3cf522..1f7d53b 100644 --- a/.github/workflows/test-dev.yml +++ b/.github/workflows/test-dev.yml @@ -39,4 +39,5 @@ jobs: - run: npm test env: ENABLE_ASAN: 1 + LSAN_OPTIONS: suppressions=${{ github.workspace }}/test/napi-leaks-suppression.txt LD_PRELOAD: /usr/lib/x86_64-linux-gnu/libasan.so.6.0.0 From fd6ced1168a4c7c95dcfbf16e193cf2dfcc7f0bb Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:28:31 +0100 Subject: [PATCH 4/9] failing the clean on Windows is normal --- test/framework.js | 17 +++++------------ test/tests.test.js | 6 +++++- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/test/framework.js b/test/framework.js index adfa8ca..cc31262 100644 --- a/test/framework.js +++ b/test/framework.js @@ -15,18 +15,11 @@ function list() { } function clean(test, stdio) { - try { - execFileSync(npx, [ - 'node-gyp', - 'clean', - `--test=${test}` - ], { stdio: stdio || 'pipe', cwd: __dirname, env }); - } catch (e) { - if (e.stdout) { - console.error(e.stdout); - } - throw e; - } + execFileSync(npx, [ + 'node-gyp', + 'clean', + `--test=${test}` + ], { stdio: stdio || 'pipe', cwd: __dirname, env }); } function configure(test, stdio, opts) { diff --git a/test/tests.test.js b/test/tests.test.js index 29c1e9f..db0e566 100644 --- a/test/tests.test.js +++ b/test/tests.test.js @@ -5,7 +5,11 @@ describe('nobind', function () { for (const t of tests) { describe(t, () => { framework.register(t); - before('clean', () => framework.clean(t)); + try { + before('clean', () => framework.clean(t)); + } catch (e) { + console.log('cleaning the build failed, module not unloaded on Windows?'); + } before('configure', () => framework.configure(t, undefined, process.env.ENABLE_ASAN && ['--debug', '--enable_asan'])); before('build', () => framework.build()); before('load', () => framework.load(t, process.env.ENABLE_ASAN && 'Debug')); From 7e3f1a9582359dd313a80065f13207dcedca4fab Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:30:07 +0100 Subject: [PATCH 5/9] add a new leak --- test/napi-leaks-suppression.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/test/napi-leaks-suppression.txt b/test/napi-leaks-suppression.txt index bbc78e2..f7cfa9b 100644 --- a/test/napi-leaks-suppression.txt +++ b/test/napi-leaks-suppression.txt @@ -25,3 +25,4 @@ leak:PagedSpace::RefillLabMain leak:OldLargeObjectSpace::AllocateRaw leak:BaselineBatchCompiler::EnqueueFunction leak:Compiler::FinalizeTurbofanCompilationJob +leak:Factory::CodeBuilder::Build From 359aa7134a9e5098c7a543f3658e2499e672a5f9 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:36:21 +0100 Subject: [PATCH 6/9] do this correctly --- test/tests.test.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/tests.test.js b/test/tests.test.js index db0e566..d7bf186 100644 --- a/test/tests.test.js +++ b/test/tests.test.js @@ -5,11 +5,13 @@ describe('nobind', function () { for (const t of tests) { describe(t, () => { framework.register(t); - try { - before('clean', () => framework.clean(t)); - } catch (e) { - console.log('cleaning the build failed, module not unloaded on Windows?'); - } + before('clean', () => { + try { + framework.clean(t); + } catch (e) { + console.log('cleaning the build failed, module not unloaded on Windows?'); + } + }); before('configure', () => framework.configure(t, undefined, process.env.ENABLE_ASAN && ['--debug', '--enable_asan'])); before('build', () => framework.build()); before('load', () => framework.load(t, process.env.ENABLE_ASAN && 'Debug')); From 2ce83cb8963de099ba8870ae90b58353d3208228 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:37:13 +0100 Subject: [PATCH 7/9] simply remove the clean from the automated testing there is no reason to do it --- test/tests.test.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/tests.test.js b/test/tests.test.js index d7bf186..b881a93 100644 --- a/test/tests.test.js +++ b/test/tests.test.js @@ -5,13 +5,6 @@ describe('nobind', function () { for (const t of tests) { describe(t, () => { framework.register(t); - before('clean', () => { - try { - framework.clean(t); - } catch (e) { - console.log('cleaning the build failed, module not unloaded on Windows?'); - } - }); before('configure', () => framework.configure(t, undefined, process.env.ENABLE_ASAN && ['--debug', '--enable_asan'])); before('build', () => framework.build()); before('load', () => framework.load(t, process.env.ENABLE_ASAN && 'Debug')); From d872583d3c40e209fa99a13fab2c05a8e0ec517f Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:48:38 +0100 Subject: [PATCH 8/9] update the changelog --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04917a6..460f757 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +### [] + +- Specify the freeing semantics for `Buffer` +- Fix a minor memory leak when calling method with incorrect number of arguments + ### [1.1.1] 2023-12-01 - - Fix [#7](https://github.com/mmomtchev/nobind/issues/7), escape the include directory path on Windows +- Fix [#7](https://github.com/mmomtchev/nobind/issues/7), escape the include directory path on Windows ## [1.1.0] 2023-11-30 From 5b66b9de6181bdef45c432979f8d10d4d7e85e97 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 21 Dec 2023 11:49:22 +0100 Subject: [PATCH 9/9] update the changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 460f757..f39d907 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### [] -- Specify the freeing semantics for `Buffer` +- Specify the memory ownership rules for `Buffer` - Fix a minor memory leak when calling method with incorrect number of arguments ### [1.1.1] 2023-12-01