Skip to content
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.

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

Fix memory leak during signing #596

Merged
merged 1 commit into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion source/http_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,12 @@ struct http_connection_binding {
/* finalizer called when node cleans up this object */
static void s_http_connection_from_manager_binding_finalize(napi_env env, void *finalize_data, void *finalize_hint) {
(void)finalize_hint;
(void)env;
struct http_connection_binding *binding = finalize_data;

if (binding->node_external != NULL) {
napi_delete_reference(env, binding->node_external);
}
Comment on lines +154 to +156
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes another leak I found when searching for uses of napi_create_reference()/napi_delete_reference())

No one is using the NodeJS bindings of our HTTP classes, as far as I know...


/* no release call, the http_client_connection_manager has already released it */
aws_mem_release(binding->allocator, binding);
}
Expand Down
4 changes: 3 additions & 1 deletion source/http_connection_manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ struct aws_http_connection_manager *aws_napi_get_http_connection_manager(

static void s_http_connection_manager_finalize(napi_env env, void *finalize_data, void *finalize_hint) {
(void)finalize_hint;
(void)env;
struct http_connection_manager_binding *binding = finalize_data;
if (binding->node_external != NULL) {
napi_delete_reference(env, binding->node_external);
}
Comment on lines +31 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes another leak I found when searching for uses of napi_create_reference()/napi_delete_reference())

No one is using the NodeJS bindings of our HTTP classes, as far as I know...

aws_mem_release(binding->allocator, binding);
}

Expand Down
43 changes: 15 additions & 28 deletions source/http_message.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,29 +79,29 @@ napi_status aws_napi_http_message_bind(napi_env env, napi_value exports) {

struct http_request_binding {
struct aws_http_message *native;
struct aws_allocator *allocator;

napi_ref node_headers;
};

/* Need a special finalizer to avoid releasing a request object we don't own */
static void s_napi_wrapped_http_request_finalize(napi_env env, void *finalize_data, void *finalize_hint) {
(void)env;
static void s_napi_http_request_finalize(napi_env env, void *finalize_data, void *finalize_hint) {
(void)finalize_hint;

struct http_request_binding *binding = finalize_data;
struct aws_allocator *allocator = binding->allocator;
struct aws_allocator *allocator = aws_napi_get_allocator();

if (binding->node_headers != NULL) {
napi_delete_reference(env, binding->node_headers);
}

aws_http_message_release(binding->native);
aws_mem_release(allocator, binding);
}

napi_status aws_napi_http_message_wrap(napi_env env, struct aws_http_message *message, napi_value *result) {

struct http_request_binding *binding =
aws_mem_calloc(aws_napi_get_allocator(), 1, sizeof(struct http_request_binding));
binding->native = message;
binding->allocator = aws_napi_get_allocator();
return aws_napi_wrap(env, &s_request_class_info, binding, s_napi_wrapped_http_request_finalize, result);
binding->native = aws_http_message_acquire(message);
return aws_napi_wrap(env, &s_request_class_info, binding, s_napi_http_request_finalize, result);
}

struct aws_http_message *aws_napi_http_message_unwrap(napi_env env, napi_value js_object) {
Expand All @@ -115,20 +115,6 @@ struct aws_http_message *aws_napi_http_message_unwrap(napi_env env, napi_value j
* Constructor
**********************************************************************************************************************/

static void s_napi_http_request_finalize(napi_env env, void *finalize_data, void *finalize_hint) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were 2 different finalizers for the same class. I combined them into 1.

(When this code was first written, the 2 different finalizers were necessary because the underlying C classes didn't have reference-counting. But now they do, so the weirdness the led to needing 2 different finalizers isn't necessary anymore)

(void)env;

struct http_request_binding *binding = finalize_data;
struct aws_allocator *allocator = finalize_hint;

if (binding->node_headers != NULL) {
napi_delete_reference(env, binding->node_headers);
}

aws_http_message_destroy(binding->native);
aws_mem_release(allocator, binding);
}

static napi_value s_request_constructor(napi_env env, const struct aws_napi_callback_info *cb_info) {

struct aws_allocator *alloc = aws_napi_get_allocator();
Expand Down Expand Up @@ -167,7 +153,7 @@ static napi_value s_request_constructor(napi_env env, const struct aws_napi_call
}

napi_value node_this = cb_info->native_this;
AWS_NAPI_CALL(env, napi_wrap(env, node_this, binding, s_napi_http_request_finalize, alloc, NULL), {
AWS_NAPI_CALL(env, napi_wrap(env, node_this, binding, s_napi_http_request_finalize, NULL, NULL), {
napi_throw_error(env, NULL, "Failed to wrap HttpRequest");
goto cleanup;
});
Expand All @@ -177,7 +163,7 @@ static napi_value s_request_constructor(napi_env env, const struct aws_napi_call
cleanup:
if (binding) {
if (binding->native) {
aws_http_message_destroy(binding->native);
aws_http_message_release(binding->native);
}
aws_mem_release(alloc, binding);
}
Expand Down Expand Up @@ -238,15 +224,16 @@ static napi_value s_request_headers_get(napi_env env, void *native_this) {

napi_value result = NULL;
if (binding->node_headers) {
/* This HTTP request already has a reference to the Node object for the headers */
AWS_NAPI_ENSURE(env, napi_get_reference_value(env, binding->node_headers, &result));
} else {
/* There is no Node object for these headers.
* Create a Node object, and a reference to it, and store that reference on this HTTP request */
struct aws_http_headers *headers = aws_http_message_get_headers(binding->native);
AWS_NAPI_ENSURE(env, aws_napi_http_headers_wrap(env, headers, &result));
AWS_NAPI_ENSURE(env, napi_create_reference(env, result, 1, &binding->node_headers));
}

/* Store the value for later */
AWS_NAPI_ENSURE(env, napi_create_reference(env, result, 1, &binding->node_headers));

return result;
Comment on lines +234 to 237
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the leak. We were creating a new napi_ref each time the headers were queried. Now, we only create a napi_ref if we didn't already have one.

}

Expand Down
2 changes: 1 addition & 1 deletion source/http_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ static void s_on_response_call(napi_env env, napi_value on_response, void *conte
}

/* clean up the response buffer */
aws_http_message_destroy(binding->response);
aws_http_message_release(binding->response);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this isn't a bugfix, just replacing a deprecated _destroy() function with a modern _release() function

binding->response = NULL;
}

Expand Down
Loading