From c4930d294c4a93b0578940a18c07bc26772b9ac5 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Thu, 23 Jan 2025 14:00:45 -0800 Subject: [PATCH] Fix memory leak during signing (#596) **Issue:** (internal ticket V1637484435) When doing SigV4A signing from aws-sdk-js-v3, memory usage gradually increases due to memory leaks. **Description of changes:** Fix leak that occurred if headers were queried repeatedly. **Future Work:** All native unit tests should check that they aren't leaking memory. I spent a few hours trying to add such tests, but couldn't get it working in a reliable way. I didn't want this to hold back the fix, so leaving it for another day. See these blog posts for hints on various techniques NodeJS has used for leak testing through the years: https://joyeecheung.github.io/blog/2024/03/17/memory-leak-testing-v8-node-js-1/, https://joyeecheung.github.io/blog/2024/03/17/memory-leak-testing-v8-node-js-2/ --- source/http_connection.c | 5 +++- source/http_connection_manager.c | 4 ++- source/http_message.c | 43 +++++++++++--------------------- source/http_stream.c | 2 +- 4 files changed, 23 insertions(+), 31 deletions(-) diff --git a/source/http_connection.c b/source/http_connection.c index 12a233c67..7f3ce9ee1 100644 --- a/source/http_connection.c +++ b/source/http_connection.c @@ -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); + } + /* no release call, the http_client_connection_manager has already released it */ aws_mem_release(binding->allocator, binding); } diff --git a/source/http_connection_manager.c b/source/http_connection_manager.c index 1d49f43c2..2cf844e92 100644 --- a/source/http_connection_manager.c +++ b/source/http_connection_manager.c @@ -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); + } aws_mem_release(binding->allocator, binding); } diff --git a/source/http_message.c b/source/http_message.c index c1c85d83f..458745db7 100644 --- a/source/http_message.c +++ b/source/http_message.c @@ -79,19 +79,20 @@ 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); } @@ -99,9 +100,8 @@ napi_status aws_napi_http_message_wrap(napi_env env, struct aws_http_message *me 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) { @@ -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) { - (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(); @@ -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; }); @@ -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); } @@ -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; } diff --git a/source/http_stream.c b/source/http_stream.c index 5921b90ce..18d87f080 100644 --- a/source/http_stream.c +++ b/source/http_stream.c @@ -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); binding->response = NULL; }