Skip to content

Fix memory leaks, error handling, and type-safety issues in N-API bindings #1422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
minggangw merged 3 commits into from
Mar 9, 2026
Merged

Fix memory leaks, error handling, and type-safety issues in N-API bindings #1422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fbe641b
Fix memory leaks, error handling, and type-safety issues in N-API bin…
minggangw Mar 6, 2026
802d4a8
Address comments
minggangw Mar 6, 2026
75bf7a0
Address comments
minggangw Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 27 additions & 15 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,23 +387,35 @@ let rcl = {

rclnodejs.init(context.handle, argv, context._domainId);

if (_rosVersionChecked) {
// no further processing required
return;
}
try {
if (_rosVersionChecked) {
// no further processing required
return;
}

const version = await getCurrentGeneratorVersion();
const forced =
version === null || compareVersions(version, generator.version(), '<');
if (forced) {
debug(
'The generator will begin to create JavaScript code from ROS IDL files...'
);
}
const version = await getCurrentGeneratorVersion();
const forced =
version === null || compareVersions(version, generator.version(), '<');
if (forced) {
debug(
'The generator will begin to create JavaScript code from ROS IDL files...'
);
}

await generator.generateAll(forced);
// TODO determine if tsd generateAll() should be here
_rosVersionChecked = true;
await generator.generateAll(forced);
// TODO determine if tsd generateAll() should be here
_rosVersionChecked = true;
} catch (error) {
try {
context.tryShutdown();
} catch (shutdownError) {
const initError =
error instanceof Error ? error : new Error(String(error));
initError.message += ` (rollback also failed: ${shutdownError.message})`;
throw initError;
}
throw error;
Comment on lines +409 to +417
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the catch block, the code mutates error.message via +=. In strict mode this can throw if error isn’t an Error object (e.g., a rejected promise with a string/primitive), which would mask the original init failure and potentially skip the intended rollback. Consider normalizing to an Error instance (or wrapping a new error with cause) before appending details, and ensure the rollback failure is reported without risking a secondary throw.

Suggested change
try {
context.tryShutdown();
} catch (shutdownError) {
error.message += ` Failed to roll back context after init failure: ${shutdownError.message}`;
}
throw error;
let toThrow = error;
try {
context.tryShutdown();
} catch (shutdownError) {
const shutdownMessage =
shutdownError instanceof Error
? shutdownError.message
: String(shutdownError);
if (error instanceof Error) {
error.message +=
` Failed to roll back context after init failure: ${shutdownMessage}`;
} else {
const originalMessage =
typeof error === 'string' ? error : String(error);
toThrow = new Error(
`${originalMessage} Failed to roll back context after init failure: ${shutdownMessage}`
);
}
}
throw toThrow;

Copilot uses AI. Check for mistakes.
}
},

/**
Expand Down
14 changes: 7 additions & 7 deletions src/rcl_action_client_bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ Napi::Value ActionSendGoalRequest(const Napi::CallbackInfo& info) {
rcl_action_send_goal_request(action_client, buffer, &sequence_number),
RCL_RET_OK, rcl_get_error_string().str);

return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}

Napi::Value ActionSendResultRequest(const Napi::CallbackInfo& info) {
Expand All @@ -149,7 +149,7 @@ Napi::Value ActionSendResultRequest(const Napi::CallbackInfo& info) {
rcl_action_send_result_request(action_client, buffer, &sequence_number),
RCL_RET_OK, rcl_get_error_string().str);

return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}

Napi::Value ActionTakeFeedback(const Napi::CallbackInfo& info) {
Expand All @@ -163,9 +163,9 @@ Napi::Value ActionTakeFeedback(const Napi::CallbackInfo& info) {

rcl_ret_t ret = rcl_action_take_feedback(action_client, buffer);
if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
Napi::Error::New(env, rcl_get_error_string().str)
.ThrowAsJavaScriptException();
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return Napi::Boolean::New(env, false);
}

Expand All @@ -186,9 +186,9 @@ Napi::Value ActionTakeStatus(const Napi::CallbackInfo& info) {

rcl_ret_t ret = rcl_action_take_status(action_client, buffer);
if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, rcl_get_error_string().str)
.ThrowAsJavaScriptException();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return Napi::Boolean::New(env, false);
}

Expand Down Expand Up @@ -247,7 +247,7 @@ Napi::Value ActionSendCancelRequest(const Napi::CallbackInfo& info) {
rcl_action_send_cancel_request(action_client, buffer, &sequence_number),
RCL_RET_OK, rcl_get_error_string().str);

return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}

#if ROS_VERSION >= 2505 // ROS2 >= Kilted
Expand Down
21 changes: 12 additions & 9 deletions src/rcl_action_server_bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ Napi::Value ActionTakeResultRequest(const Napi::CallbackInfo& info) {
return js_obj;
}

free(header);
return env.Undefined();
}

Expand All @@ -148,6 +149,7 @@ Napi::Value ActionTakeGoalRequest(const Napi::CallbackInfo& info) {
return js_obj;
}

free(header);
return env.Undefined();
}

Expand Down Expand Up @@ -221,14 +223,14 @@ Napi::Value ActionTakeGoalResponse(const Napi::CallbackInfo& info) {
free(header);

if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, rcl_get_error_string().str)
.ThrowAsJavaScriptException();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return env.Undefined();
}

if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}
Comment on lines 232 to 234
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sequence_number is int64_t but is returned as a JS Number (double). While this avoids the previous 32-bit narrowing, it still cannot represent all 64-bit values exactly beyond Number.MAX_SAFE_INTEGER, and JS uses these sequence numbers to correlate action responses. Consider using Napi::BigInt for a lossless representation (or documenting/enforcing a safe range).

Copilot uses AI. Check for mistakes.
return env.Undefined();
}
Expand All @@ -250,14 +252,14 @@ Napi::Value ActionTakeCancelResponse(const Napi::CallbackInfo& info) {
free(header);

if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, rcl_get_error_string().str)
.ThrowAsJavaScriptException();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return env.Undefined();
}

if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}
return env.Undefined();
}
Expand All @@ -279,14 +281,14 @@ Napi::Value ActionTakeResultResponse(const Napi::CallbackInfo& info) {
free(header);

if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, rcl_get_error_string().str)
.ThrowAsJavaScriptException();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return env.Undefined();
}

if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}
return env.Undefined();
}
Expand Down Expand Up @@ -463,6 +465,7 @@ Napi::Value ActionTakeCancelRequest(const Napi::CallbackInfo& info) {
return js_obj;
}

free(header);
return env.Undefined();
}

Expand Down
4 changes: 2 additions & 2 deletions src/rcl_client_bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ Napi::Value SendRequest(const Napi::CallbackInfo& info) {
THROW_ERROR_IF_NOT_EQUAL(rcl_send_request(client, buffer, &sequence_number),
RCL_RET_OK, rcl_get_error_string().str);

return Napi::Number::New(env, static_cast<uint32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}

Napi::Value RclTakeResponse(const Napi::CallbackInfo& info) {
Expand All @@ -104,7 +104,7 @@ Napi::Value RclTakeResponse(const Napi::CallbackInfo& info) {
int64_t sequence_number = header.request_id.sequence_number;

if (ret == RCL_RET_OK) {
return Napi::Number::New(env, static_cast<uint32_t>(sequence_number));
return Napi::Number::New(env, static_cast<double>(sequence_number));
}

rcl_reset_error();
Expand Down
1 change: 1 addition & 0 deletions src/rcl_service_bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ Napi::Value RclTakeRequest(const Napi::CallbackInfo& info) {
return js_obj;
}

free(header);
return env.Undefined();
}

Expand Down
45 changes: 24 additions & 21 deletions src/rcl_subscription_bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

#include <cstdio>
#include <memory>
#include <rcpputils/scope_exit.hpp>
// NOLINTNEXTLINE
#include <string>

#include "macros.h"
Expand Down Expand Up @@ -108,12 +110,6 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
rcl_ret_t ret = rcl_subscription_options_set_content_filter_options(
expression.c_str(), argc, (const char**)argv, &subscription_ops);

if (ret != RCL_RET_OK) {
std::string error_string = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
}

if (argc) {
for (int i = 0; i < argc; i++) {
free(argv[i]);
Expand All @@ -122,7 +118,10 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
}

if (ret != RCL_RET_OK) {
std::string error_string = rcl_get_error_string().str;
rcl_reset_error();
free(subscription);
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
return env.Undefined();
}
}
Expand All @@ -137,8 +136,8 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
if (ret != RCL_RET_OK) {
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
free(subscription);
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return env.Undefined();
}

Expand All @@ -157,9 +156,9 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {

return js_obj;
} else {
Napi::Error::New(env, GetErrorMessageAndClear())
.ThrowAsJavaScriptException();
std::string error_msg = GetErrorMessageAndClear();
free(subscription);
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return env.Undefined();
}
}
Expand Down Expand Up @@ -194,10 +193,16 @@ Napi::Value RclTakeRaw(const Napi::CallbackInfo& info) {
return env.Undefined();
}

RCPPUTILS_SCOPE_EXIT({
rcl_ret_t fini_ret = rmw_serialized_message_fini(&msg);
if (fini_ret != RCL_RET_OK) {
rcl_reset_error();
}
});

Napi::Buffer<char> buffer = Napi::Buffer<char>::Copy(
env, reinterpret_cast<char*>(msg.buffer), msg.buffer_length);
THROW_ERROR_IF_NOT_EQUAL(rmw_serialized_message_fini(&msg), RCL_RET_OK,
"Failed to deallocate message buffer");

return buffer;
}

Expand Down Expand Up @@ -369,6 +374,14 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) {
return env.Undefined();
}

RCPPUTILS_SCOPE_EXIT({
rcl_ret_t fini_ret =
rcl_subscription_content_filter_options_fini(subscription, &options);
if (fini_ret != RCL_RET_OK) {
rcl_reset_error();
}
});
Comment on lines +377 to +383
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetContentFilter now finalizes rcl_subscription_content_filter_options_t via RCPPUTILS_SCOPE_EXIT, but the return code from rcl_subscription_content_filter_options_fini() is no longer checked and any error string is no longer surfaced to JS. This is a behavioral regression vs the prior explicit cleanup path and can also leave the global RCL error state uncleared if *_fini fails. Consider finalizing in a way that preserves exception safety but still checks/clears errors on the normal return path.

Copilot uses AI. Check for mistakes.

// Create result object
Napi::Object result = Napi::Object::New(env);
result.Set(
Expand All @@ -387,16 +400,6 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) {
}
result.Set("parameters", parameters);

// Cleanup
rcl_ret_t fini_ret =
rcl_subscription_content_filter_options_fini(subscription, &options);
if (fini_ret != RCL_RET_OK) {
std::string error_msg = rcl_get_error_string().str;
rcl_reset_error();
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
return env.Undefined();
}

return result;
}

Expand Down
36 changes: 36 additions & 0 deletions test/test-init-shutdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
'use strict';

const assert = require('assert');
const sinon = require('sinon');
const rclnodejs = require('../index.js');
const generator = require('../rosidl_gen/index.js');

describe('rclnodejs init and shutdown test suite', function () {
this.timeout(60 * 1000);
Expand Down Expand Up @@ -99,6 +101,40 @@ describe('rclnodejs init and shutdown test suite', function () {
}, 'shutting rclnodejs down twice should not cause an error to be thrown');
});

it('rolls back context when generator init step fails', async function () {
rclnodejs.shutdownAll();

const generateAllStub = sinon
.stub(generator, 'generateAll')
.rejects(new Error('generator failed'));

try {
const indexPath = require.resolve('../index.js');
delete require.cache[indexPath];
const freshRclnodejs = require('../index.js');

const failedContext = new freshRclnodejs.Context();

await assert.rejects(async () => {
await freshRclnodejs.init(failedContext);
}, /generator failed/);
assert.ok(freshRclnodejs.isShutdown(failedContext));

generateAllStub.restore();

const recoveredContext = new freshRclnodejs.Context();
await assert.doesNotReject(async () => {
await freshRclnodejs.init(recoveredContext);
});
freshRclnodejs.shutdown(recoveredContext);
freshRclnodejs.removeSignalHandlers();
} finally {
if (generateAllStub.restore) {
generateAllStub.restore();
}
}
});

it('rclnodejs create node without init should fail', async function () {
assert.throws(
() => {
Expand Down
Loading