automated asan testing & fix some memory leaks

.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

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
 

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
 

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<uint8_t *, size_t>
 using Buffer = std::pair<uint8_t *, size_t>;
 
+// 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> {
   Buffer val_;
 
@@ -27,15 +33,46 @@ template <> class FromJS<Buffer> {
   }
 
   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 <const ReturnAttribute &RETATTR> class ToJS<Buffer, RETATTR> {
   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<uint8_t>::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<uint8_t>::Copy(env_, val_.first, val_.second);
+    delete[] val->data;
+    return buffer;
+#else
+    // Node-API supports external buffers (Node.js)
+    return Napi::Buffer<uint8_t>::New(env_, val_.first, val_.second, [](Napi::Env, uint8_t *data) { delete[] data; });
+#endif
+  }
+
+  ToJS(const ToJS &) = delete;
+  ToJS(ToJS &&) = delete;
 };
 
 } // namespace Typemap

include/nofunction.h

@@ -96,7 +96,12 @@ inline Napi::Value FunctionWrapperAsync(const Napi::CallbackInfo &info,
     auto tasklet =
         new FunctionWrapperTasklet<RETATTR, FUNC, RETURN, ARGS...>(env, deferred, {FromJSArgs<ARGS>(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) {

include/noobject.h

@@ -237,7 +237,12 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
       // https://en.cppreference.com/w/cpp/language/list_initialization
       auto tasklet = new MethodWrapperTasklet<RETATTR, BASE, FUNC, RETURN, ARGS...>(env, deferred, self,
                                                                                     {FromJSArgs<ARGS>(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) {

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'
+          ]
+        }        
+      }]
+    ]
   }
 }

test/fixtures/buffer.cc

@@ -2,6 +2,7 @@
 #include <stdexcept>
 
 // 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<uint32_t *>(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<uint32_t *>(buf);
   for (size_t i = 0; i < len * sizeof(uint8_t) / sizeof(uint32_t); i++) {

test/framework.js

@@ -4,13 +4,24 @@ 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'))
     .filter((t) => t.endsWith('.cc'))
     .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,

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

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 <configure|build|run> <test>');
@@ -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();

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);
     });
   }