diff --git a/.github/workflows/test-dev.yml b/.github/workflows/test-dev.yml index 03bf0b1..1f7d53b 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,20 @@ 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 18.x + uses: actions/setup-node@v3 + with: + node-version: 18.x + - run: npm install + - 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 04917a6..f39d907 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 memory ownership rules 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 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..cc31262 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,14 @@ function list() { .map((t) => t.split('.')[0]); } +function clean(test, stdio) { + execFileSync(npx, [ + 'node-gyp', + 'clean', + `--test=${test}` + ], { stdio: stdio || 'pipe', cwd: __dirname, env }); +} + function configure(test, stdio, opts) { try { let include; @@ -26,7 +37,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 +50,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 +63,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..f7cfa9b --- /dev/null +++ b/test/napi-leaks-suppression.txt @@ -0,0 +1,28 @@ +# 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 +leak:Factory::CodeBuilder::Build 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..b881a93 100644 --- a/test/tests.test.js +++ b/test/tests.test.js @@ -5,9 +5,9 @@ describe('nobind', function () { for (const t of tests) { describe(t, () => { framework.register(t); - before('configure', () => framework.configure(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); }); }