fix: buffer overflows (#684)

Fix leak in CreateNode/ShadowNode

For some reason, assigning the object wrapping `RclHandle` to a new persistent handle causes the weak callback to not get called, and the wrapped `RclHandle` is lost. My theory is that now that it has a strong persistent handle keeping it alive, v8 decides that the weak persistent handle is not needed anymore and disposes it. The fix is to simply remove the `handle` accessors, since the `RclHandle`'s life is now handled by v8, it should be automatically cleaned up when there is no more reference to the `ShadowNode`.

* add cpp test artifacts to .gitignore

* fix possible buffer overflows and use after stack scope

* add helper scripts and instructions to run tests with asan

* fix cpplint errors

* make node.handle readonly; remove corrupted handle destruction test

Fix #674

Author: Teo Koon Peng

.gitignore

@@ -21,3 +21,7 @@ install
 log
 .vscode
 .project
+
+# test build artifacts
+/test/cpp/add_two_ints_client
+/test/cpp/listener

README.md

@@ -5,11 +5,11 @@
 | develop | [![Build Status](https://travis-ci.org/RobotWebTools/rclnodejs.svg?branch=develop)](https://travis-ci.org/RobotWebTools/rclnodejs) | [![macOS Build Status](https://circleci.com/gh/RobotWebTools/rclnodejs/tree/develop.svg?style=shield)](https://circleci.com/gh/RobotWebTools/rclnodejs) | [![Build status](https://ci.appveyor.com/api/projects/status/upbc7tavdag1aa5e/branch/develop?svg=true)](https://ci.appveyor.com/project/minggangw/rclnodejs/branch/develop) |
 | master  | [![Build Status](https://travis-ci.org/RobotWebTools/rclnodejs.svg?branch=master)](https://travis-ci.org/RobotWebTools/rclnodejs)  | [![macOS Build Status](https://circleci.com/gh/RobotWebTools/rclnodejs/tree/master.svg?style=shield)](https://circleci.com/gh/RobotWebTools/rclnodejs)  |  [![Build status](https://ci.appveyor.com/api/projects/status/upbc7tavdag1aa5e/branch/master?svg=true)](https://ci.appveyor.com/project/minggangw/rclnodejs/branch/master)  |
 
-`rclnodejs` is a Node.js client for the Robot Operating System (ROS 2). It provides a simple and easy JavaScript API for ROS 2 programming. TypeScript declarations are included to support use of rclnodejs in TypeScript projects. 
+`rclnodejs` is a Node.js client for the Robot Operating System (ROS 2). It provides a simple and easy JavaScript API for ROS 2 programming. TypeScript declarations are included to support use of rclnodejs in TypeScript projects.
 
-Here's an example for how to create a ROS 2 node that publishes a string message in a few lines of JavaScript. 
+Here's an example for how to create a ROS 2 node that publishes a string message in a few lines of JavaScript.
 
-``` JavaScript
+```JavaScript
 const rclnodejs = require('rclnodejs');
 rclnodejs.init().then(() => {
   const node = rclnodejs.createNode('publisher_example_node');
@@ -21,27 +21,30 @@ rclnodejs.init().then(() => {
 
 ## Prerequisites
 
-**Node.js**  
-* [Node.js](https://nodejs.org/en/) version between 8.12 - 12.x.
+**Node.js**
 
-**ROS 2 SDK**  
-* See the ROS 2 SDK [Installation Guide](https://index.ros.org/doc/ros2/Installation/) for details.
-* *** DON'T FORGET TO [SOURCE THE ROS 2 STARTUP FILES](https://index.ros.org/doc/ros2/Tutorials/Configuring-ROS2-Environment/#source-the-setup-files) ***
+- [Node.js](https://nodejs.org/en/) version between 8.12 - 12.x.
+
+**ROS 2 SDK**
+
+- See the ROS 2 SDK [Installation Guide](https://index.ros.org/doc/ros2/Installation/) for details.
+- **_ DON'T FORGET TO [SOURCE THE ROS 2 STARTUP FILES](https://index.ros.org/doc/ros2/Tutorials/Configuring-ROS2-Environment/#source-the-setup-files) _**
 
 ## Install rclnodejs
 
-Install the rclnodejs version that is compatible with your version of ROS 2 (see table below). 
- 
+Install the rclnodejs version that is compatible with your version of ROS 2 (see table below).
+
 Run the following command for the most current version of rclnodejs
 
-``` bash
+```bash
 npm i rclnodejs
 ```
+
 or to install a specific version of rclnodejs use
-``` bash
+
+```bash
 npm i [email protected]
 ```
-#### RCLNODEJS - ROS 2 Version Compatibility 
 
 | RCLNODEJS Version | Compatible ROS 2 Release |
 | :---------------: | :-----------: |
@@ -97,7 +100,6 @@ The benefits of using TypeScript become evident when working with more complex u
    }
 ```
 
-
 ## Build from Scratch
 
 ### Get ready for ROS 2
@@ -201,6 +203,36 @@ This is caused by a bug in `ref` which happens when you `require` it multiple ti
 
 If it is required to use Jest, a solution would be to fork `ref` and use npm shrinkwrap to installed a patched version.
 
+### Running with ASAN
+
+#### Linux
+
+To run with google's AddressSanitizer tool, build with `-fsanitize=address` flag,
+
+```sh
+CXXFLAGS=-fsanitize=address node-gyp build --debug
+```
+
+ASAN needs to be loaded at the start of the process, since rclnodejs is a dynamically loaded library, it will not do so by default. To workaround this, run node with `LD_PRELOAD` to force ASAN to be loaded.
+
+```sh
+LD_PRELOAD=$(g++ -print-file-name=libasan.so) node node_modules/.bin/mocha test/test-publisher.js
+```
+
+Due to v8's garbage collector, there may be false positives in the leak test, to remove them as much as possible, there is a simple helper script to run gc on exit. To use it, the `--expose-gc` flag needs to be set in node, then run mocha with `-r test/gc-on-exit.js` e.g.
+
+```sh
+LD_PRELOAD=$(g++ -print-file-name=libasan.so) node --expose-gc node_modules/.bin/mocha -r test/gc-on-exit.js test/test-publisher.js
+```
+
+**Note**: Tests that forks the current process like `test-array.js` will not run gc when they exit. They may report many false positive leaks.
+
+ASAN may report leaks in ref-napi and other modules, there is a suppression file you can use to hide them
+
+```sh
+LSAN_OPTIONS=suppressions=suppr.txt node --expose-gc node_modules/.bin/mocha -r test/gc-on-exit.js test/test-publisher.js
+```
+
 ## Get Involved
 
 ### Contributing

index.js

@@ -52,7 +52,7 @@ const {
 
 function inherits(target, source) {
   let properties = Object.getOwnPropertyNames(source.prototype);
-  properties.forEach(property => {
+  properties.forEach((property) => {
     target.prototype[property] = source.prototype[property];
   });
 }
@@ -86,7 +86,6 @@ function getCurrentGeneratorVersion() {
  * @exports rclnodejs
  */
 let rcl = {
-  
   _rosVersionChecked: false,
 
   // Map<Context,Array<Node>
@@ -198,13 +197,10 @@ let rcl = {
     let handle = rclnodejs.createNode(nodeName, namespace, context.handle());
     let node = new rclnodejs.ShadowNode();
     node.handle = handle;
+    Object.defineProperty(node, 'handle', { configurable: false, writable: false }); // make read-only
     node.context = context;
     node.init(nodeName, namespace, context, options);
-    debug(
-      'Finish initializing node, name = %s and namespace = %s.',
-      nodeName,
-      namespace
-    );
+    debug('Finish initializing node, name = %s and namespace = %s.', nodeName, namespace);
 
     this._contextToNodeArrayMap.get(context).push(node);
     return node;
@@ -217,12 +213,11 @@ let rcl = {
    * @return {Promise<undefined>} A Promise.
    */
   init(context = Context.defaultContext(), argv = process.argv) {
-    
     return new Promise((resolve, reject) => {
       // check if context has already been initialized
       if (this._contextToNodeArrayMap.has(context)) {
         throw new Error('The context has already been initialized.');
-      };
+      }
 
       // check argv for correct value and state
       if (!Array.isArray(argv)) {
@@ -242,14 +237,10 @@ let rcl = {
       }
 
       getCurrentGeneratorVersion()
-        .then(version => {
-          let forced =
-            version === null || 
-            compareVersions(version, generator.version()) === -1;
+        .then((version) => {
+          let forced = version === null || compareVersions(version, generator.version()) === -1;
           if (forced) {
-            debug(
-              'The generator will begin to create JavaScript code from ROS IDL files...'
-            );
+            debug('The generator will begin to create JavaScript code from ROS IDL files...');
           }
 
           generator
@@ -258,11 +249,11 @@ let rcl = {
               this._rosVersionChecked = true;
               resolve();
             })
-            .catch(e => {
+            .catch((e) => {
               reject(e);
             });
         })
-        .catch(e => {
+        .catch((e) => {
           reject(e);
         });
     });
@@ -311,20 +302,20 @@ let rcl = {
     }
 
     // shutdown and remove all nodes assigned to context
-    this._contextToNodeArrayMap.get(context).forEach(node => {
+    this._contextToNodeArrayMap.get(context).forEach((node) => {
       node.stopSpinning();
       node.destroy();
     });
     this._contextToNodeArrayMap.delete(context);
-    
+
     // shutdown context
     if (context === Context.defaultContext()) {
       Context.shutdownDefaultContext();
     } else {
       context.shutdown();
     }
   },
-  
+
   /**
    * A predictate for testing if a context has been shutdown.
    * @param {Context} [context=defaultContext] - The context to inspect
@@ -361,7 +352,7 @@ let rcl = {
           debug('Finish regeneration.');
           resolve();
         })
-        .catch(e => {
+        .catch((e) => {
           reject(e);
         });
     });
@@ -424,7 +415,7 @@ process.on('SIGINT', () => {
 
 module.exports = rcl;
 
-// The following statements are located here to work around a 
+// The following statements are located here to work around a
 // circular dependency issue occuring in rate.js
 const Node = require('./lib/node.js');
 const TimeSource = require('./lib/time_source.js');

src/addon.cpp

@@ -25,18 +25,15 @@
 void InitModule(v8::Local<v8::Object> exports) {
   v8::Local<v8::Context> context = exports->GetIsolate()->GetCurrentContext();
 
-  for (uint32_t i = 0;
-       i < rclnodejs::GetBindingMethodsCount(rclnodejs::binding_methods); i++) {
+  for (uint32_t i = 0; i < rclnodejs::binding_methods.size(); i++) {
     Nan::Set(
         exports, Nan::New(rclnodejs::binding_methods[i].name).ToLocalChecked(),
         Nan::New<v8::FunctionTemplate>(rclnodejs::binding_methods[i].function)
             ->GetFunction(context)
             .ToLocalChecked());
   }
 
-  for (uint32_t i = 0;
-       i < rclnodejs::GetBindingMethodsCount(rclnodejs::action_binding_methods);
-       i++) {
+  for (uint32_t i = 0; i < rclnodejs::action_binding_methods.size(); i++) {
     Nan::Set(
         exports,
         Nan::New(rclnodejs::action_binding_methods[i].name).ToLocalChecked(),

src/rcl_action_bindings.cpp

@@ -22,6 +22,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "handle_manager.hpp"
 #include "macros.hpp"
@@ -761,7 +762,7 @@ NAN_METHOD(ActionGetNamesAndTypes) {
   info.GetReturnValue().Set(result_list);
 }
 
-BindingMethod action_binding_methods[] = {
+std::vector<BindingMethod> action_binding_methods = {
     {"actionCreateClient", ActionCreateClient},
     {"actionCreateServer", ActionCreateServer},
     {"actionServerIsAvailable", ActionServerIsAvailable},

src/rcl_action_bindings.hpp

@@ -18,13 +18,13 @@
 #include <nan.h>
 #include <rcl/rcl.h>
 
-#include <string>
+#include <vector>
 
 #include "rcl_bindings.hpp"
 
 namespace rclnodejs {
 
-extern BindingMethod action_binding_methods[];
+extern std::vector<BindingMethod> action_binding_methods;
 
 }  // namespace rclnodejs
 

src/rcl_bindings.cpp

@@ -38,6 +38,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "handle_manager.hpp"
 #include "macros.hpp"
@@ -91,10 +92,10 @@ NAN_METHOD(Init) {
     argv = reinterpret_cast<char**>(malloc(argc * sizeof(char*)));
     for (int i = 0; i < argc; i++) {
       Nan::MaybeLocal<v8::Value> jsElement = Nan::Get(jsArgv, i);
-      char* arg = *Nan::Utf8String(jsElement.ToLocalChecked());
-      int len = std::strlen(arg) + 1;
+      Nan::Utf8String utf8_arg(jsElement.ToLocalChecked());
+      int len = utf8_arg.length() + 1;
       argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char*)));
-      snprintf(argv[i], len, "%s", arg);
+      snprintf(argv[i], len, "%s", *utf8_arg);
     }
   }
 
@@ -1691,15 +1692,7 @@ NAN_METHOD(ServiceServerIsAvailable) {
   info.GetReturnValue().Set(result);
 }
 
-uint32_t GetBindingMethodsCount(BindingMethod* methods) {
-  uint32_t count = 0;
-  while (methods[count].function) {
-    count++;
-  }
-  return count;
-}
-
-BindingMethod binding_methods[] = {
+std::vector<BindingMethod> binding_methods = {
     {"init", Init},
     {"createNode", CreateNode},
     {"getParameterOverrides", GetParameterOverrides},

src/rcl_bindings.hpp

@@ -21,6 +21,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 namespace rclnodejs {
 
@@ -36,11 +37,9 @@ extern rcl_guard_condition_t* g_sigint_gc;
 void ExtractNamesAndTypes(rcl_names_and_types_t names_and_types,
                           v8::Local<v8::Array>* result_list);
 
-uint32_t GetBindingMethodsCount(BindingMethod* methods);
-
 std::unique_ptr<rmw_qos_profile_t> GetQoSProfile(v8::Local<v8::Value> qos);
 
-extern BindingMethod binding_methods[];
+extern std::vector<BindingMethod> binding_methods;
 
 }  // namespace rclnodejs
 

src/shadow_node.cpp

@@ -25,20 +25,14 @@ namespace rclnodejs {
 
 Nan::Persistent<v8::Function> ShadowNode::constructor;
 
-ShadowNode::ShadowNode()
-    : handle_manager_(std::make_unique<HandleManager>()), rcl_handle_(nullptr) {
+ShadowNode::ShadowNode() : handle_manager_(std::make_unique<HandleManager>()) {
   executor_ = std::make_unique<Executor>(handle_manager_.get(), this);
-  rcl_handle_.reset(new Nan::Persistent<v8::Object>());
 }
 
 ShadowNode::~ShadowNode() {
   Nan::HandleScope scope;
 
   StopRunning();
-  if (!rcl_handle_->IsEmpty()) {
-    RclHandle* handle = RclHandle::Unwrap<RclHandle>(Nan::New(*rcl_handle_));
-    handle->Reset();
-  }
 }
 
 void ShadowNode::Init(v8::Local<v8::Object> exports) {
@@ -49,8 +43,6 @@ void ShadowNode::Init(v8::Local<v8::Object> exports) {
   tpl->SetClassName(Nan::New("ShadowNode").ToLocalChecked());
   tpl->InstanceTemplate()->SetInternalFieldCount(1);
 
-  Nan::SetAccessor(tpl->InstanceTemplate(), Nan::New("handle").ToLocalChecked(),
-                   HandleGetter, HandleSetter);
   Nan::SetPrototypeMethod(tpl, "start", Start);
   Nan::SetPrototypeMethod(tpl, "stop", Stop);
   Nan::SetPrototypeMethod(tpl, "syncHandles", SyncHandles);
@@ -63,21 +55,6 @@ void ShadowNode::Init(v8::Local<v8::Object> exports) {
            tpl->GetFunction(context).ToLocalChecked());
 }
 
-NAN_GETTER(ShadowNode::HandleGetter) {
-  auto* me = ShadowNode::Unwrap<ShadowNode>(info.Holder());
-
-  if (!me->rcl_handle()->IsEmpty())
-    info.GetReturnValue().Set(Nan::New(*(me->rcl_handle())));
-  else
-    info.GetReturnValue().Set(Nan::Undefined());
-}
-
-NAN_SETTER(ShadowNode::HandleSetter) {
-  auto* me = ShadowNode::Unwrap<ShadowNode>(info.Holder());
-  auto obj = Nan::To<v8::Object>(value).ToLocalChecked();
-  if (obj->InternalFieldCount() > 0) me->rcl_handle()->Reset(obj);
-}
-
 void ShadowNode::StopRunning() {
   executor_->Stop();
   handle_manager_->ClearHandles();