From 753b0544fdbea8618f2a98fe86eab5419f51c0ce Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 3 Apr 2017 15:31:53 -0700 Subject: [PATCH 1/2] Properly unref some slices in Node glue code --- src/node/ext/call.cc | 17 +++++++++++++---- src/node/ext/node_grpc.cc | 12 +++++++++--- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/node/ext/call.cc b/src/node/ext/call.cc index 244546d3d78..5c311158542 100644 --- a/src/node/ext/call.cc +++ b/src/node/ext/call.cc @@ -574,6 +574,14 @@ void Call::CompleteBatch(bool is_final_op) { } NAN_METHOD(Call::New) { + /* Arguments: + * 0: Channel to make the call on + * 1: Method + * 2: Deadline + * 3: host + * 4: parent Call + * 5: propagation flags + */ if (info.IsConstructCall()) { Call *call; if (info[0]->IsExternal()) { @@ -618,25 +626,26 @@ NAN_METHOD(Call::New) { double deadline = Nan::To(info[2]).FromJust(); grpc_channel *wrapped_channel = channel->GetWrappedChannel(); grpc_call *wrapped_call; + grpc_slice method = CreateSliceFromString( + Nan::To(info[1]).ToLocalChecked()); if (info[3]->IsString()) { grpc_slice *host = new grpc_slice; *host = CreateSliceFromString( Nan::To(info[3]).ToLocalChecked()); wrapped_call = grpc_channel_create_call( wrapped_channel, parent_call, propagate_flags, - GetCompletionQueue(), CreateSliceFromString( - Nan::To(info[1]).ToLocalChecked()), + GetCompletionQueue(), method, host, MillisecondsToTimespec(deadline), NULL); delete host; } else if (info[3]->IsUndefined() || info[3]->IsNull()) { wrapped_call = grpc_channel_create_call( wrapped_channel, parent_call, propagate_flags, - GetCompletionQueue(), CreateSliceFromString( - Nan::To(info[1]).ToLocalChecked()), + GetCompletionQueue(), method, NULL, MillisecondsToTimespec(deadline), NULL); } else { return Nan::ThrowTypeError("Call's fourth argument must be a string"); } + grpc_slice_unref(method); call = new Call(wrapped_call); Nan::Set(info.This(), Nan::New("channel_").ToLocalChecked(), channel_object); diff --git a/src/node/ext/node_grpc.cc b/src/node/ext/node_grpc.cc index 95e273f8ac9..122e5e63ee9 100644 --- a/src/node/ext/node_grpc.cc +++ b/src/node/ext/node_grpc.cc @@ -286,8 +286,10 @@ NAN_METHOD(MetadataKeyIsLegal) { "headerKeyIsLegal's argument must be a string"); } Local key = Nan::To(info[0]).ToLocalChecked(); + grpc_slice slice = CreateSliceFromString(key); info.GetReturnValue().Set(static_cast( - grpc_header_key_is_legal(CreateSliceFromString(key)))); + grpc_header_key_is_legal(slice))); + grpc_slice_unref(slice); } NAN_METHOD(MetadataNonbinValueIsLegal) { @@ -296,8 +298,10 @@ NAN_METHOD(MetadataNonbinValueIsLegal) { "metadataNonbinValueIsLegal's argument must be a string"); } Local value = Nan::To(info[0]).ToLocalChecked(); + grpc_slice slice = CreateSliceFromString(value); info.GetReturnValue().Set(static_cast( - grpc_header_nonbin_value_is_legal(CreateSliceFromString(value)))); + grpc_header_nonbin_value_is_legal(slice))); + grpc_slice_unref(slice); } NAN_METHOD(MetadataKeyIsBinary) { @@ -306,8 +310,10 @@ NAN_METHOD(MetadataKeyIsBinary) { "metadataKeyIsLegal's argument must be a string"); } Local key = Nan::To(info[0]).ToLocalChecked(); + grpc_slice slice = CreateSliceFromString(key); info.GetReturnValue().Set(static_cast( - grpc_is_binary_header(CreateSliceFromString(key)))); + grpc_is_binary_header(slice))); + grpc_slice_unref(slice); } static grpc_ssl_roots_override_result get_ssl_roots_override( From 77e06b2f8617f190fbd14e47238ce8d304118d2c Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 6 Apr 2017 10:37:44 -0700 Subject: [PATCH 2/2] Node: fix leak of sent metadata --- src/node/ext/call.cc | 48 +++++++++++++++++++++++--------- src/node/ext/call.h | 2 ++ src/node/ext/call_credentials.cc | 2 ++ 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/node/ext/call.cc b/src/node/ext/call.cc index 5c311158542..c77d6a3379b 100644 --- a/src/node/ext/call.cc +++ b/src/node/ext/call.cc @@ -99,7 +99,6 @@ Local nanErrorWithCode(const char *msg, grpc_call_error code) { bool CreateMetadataArray(Local metadata, grpc_metadata_array *array) { HandleScope scope; - grpc_metadata_array_init(array); Local keys = Nan::GetOwnPropertyNames(metadata).ToLocalChecked(); for (unsigned int i = 0; i < keys->Length(); i++) { Local current_key = Nan::To( @@ -111,18 +110,20 @@ bool CreateMetadataArray(Local metadata, grpc_metadata_array *array) { array->capacity += Local::Cast(value_array)->Length(); } array->metadata = reinterpret_cast( - gpr_malloc(array->capacity * sizeof(grpc_metadata))); + gpr_zalloc(array->capacity * sizeof(grpc_metadata))); for (unsigned int i = 0; i < keys->Length(); i++) { Local current_key(Nan::To(keys->Get(i)).ToLocalChecked()); Local values = Local::Cast( Nan::Get(metadata, current_key).ToLocalChecked()); - grpc_slice key_slice = grpc_slice_intern(CreateSliceFromString(current_key)); + grpc_slice key_slice = CreateSliceFromString(current_key); + grpc_slice key_intern_slice = grpc_slice_intern(key_slice); + grpc_slice_unref(key_slice); for (unsigned int j = 0; j < values->Length(); j++) { Local value = Nan::Get(values, j).ToLocalChecked(); grpc_metadata *current = &array->metadata[array->count]; - current->key = key_slice; + current->key = key_intern_slice; // Only allow binary headers for "-bin" keys - if (grpc_is_binary_header(key_slice)) { + if (grpc_is_binary_header(key_intern_slice)) { if (::node::Buffer::HasInstance(value)) { current->value = CreateSliceFromBuffer(value); } else { @@ -142,6 +143,14 @@ bool CreateMetadataArray(Local metadata, grpc_metadata_array *array) { return true; } +void DestroyMetadataArray(grpc_metadata_array *array) { + for (size_t i = 0; i < array->count; i++) { + // Don't unref keys because they are interned + grpc_slice_unref(array->metadata[i].value); + } + grpc_metadata_array_destroy(array); +} + Local ParseMetadata(const grpc_metadata_array *metadata_array) { EscapableHandleScope scope; grpc_metadata *metadata_elements = metadata_array->metadata; @@ -179,6 +188,12 @@ Op::~Op() { class SendMetadataOp : public Op { public: + SendMetadataOp() { + grpc_metadata_array_init(&send_metadata); + } + ~SendMetadataOp() { + DestroyMetadataArray(&send_metadata); + } Local GetNodeValue() const { EscapableHandleScope scope; return scope.Escape(Nan::True()); @@ -187,17 +202,16 @@ class SendMetadataOp : public Op { if (!value->IsObject()) { return false; } - grpc_metadata_array array; MaybeLocal maybe_metadata = Nan::To(value); if (maybe_metadata.IsEmpty()) { return false; } if (!CreateMetadataArray(maybe_metadata.ToLocalChecked(), - &array)) { + &send_metadata)) { return false; } - out->data.send_initial_metadata.count = array.count; - out->data.send_initial_metadata.metadata = array.metadata; + out->data.send_initial_metadata.count = send_metadata.count; + out->data.send_initial_metadata.metadata = send_metadata.metadata; return true; } bool IsFinalOp() { @@ -207,6 +221,8 @@ class SendMetadataOp : public Op { std::string GetTypeString() const { return "send_metadata"; } + private: + grpc_metadata_array send_metadata; }; class SendMessageOp : public Op { @@ -272,8 +288,12 @@ class SendClientCloseOp : public Op { class SendServerStatusOp : public Op { public: + SendServerStatusOp() { + grpc_metadata_array_init(&status_metadata); + } ~SendServerStatusOp() { grpc_slice_unref(details); + DestroyMetadataArray(&status_metadata); } Local GetNodeValue() const { EscapableHandleScope scope; @@ -313,12 +333,13 @@ class SendServerStatusOp : public Op { } Local details = Nan::To( maybe_details.ToLocalChecked()).ToLocalChecked(); - grpc_metadata_array array; - if (!CreateMetadataArray(metadata, &array)) { + if (!CreateMetadataArray(metadata, &status_metadata)) { return false; } - out->data.send_status_from_server.trailing_metadata_count = array.count; - out->data.send_status_from_server.trailing_metadata = array.metadata; + out->data.send_status_from_server.trailing_metadata_count = + status_metadata.count; + out->data.send_status_from_server.trailing_metadata = + status_metadata.metadata; out->data.send_status_from_server.status = static_cast(code); this->details = CreateSliceFromString(details); @@ -335,6 +356,7 @@ class SendServerStatusOp : public Op { private: grpc_slice details; + grpc_metadata_array status_metadata; }; class GetMetadataOp : public Op { diff --git a/src/node/ext/call.h b/src/node/ext/call.h index cffff00fce4..fe2abab9ee3 100644 --- a/src/node/ext/call.h +++ b/src/node/ext/call.h @@ -58,6 +58,8 @@ v8::Local ParseMetadata(const grpc_metadata_array *metadata_array); bool CreateMetadataArray(v8::Local metadata, grpc_metadata_array *array); +void DestroyMetadataArray(grpc_metadata_array *array); + /* Wrapper class for grpc_call structs. */ class Call : public Nan::ObjectWrap { public: diff --git a/src/node/ext/call_credentials.cc b/src/node/ext/call_credentials.cc index afcc363131e..5bd4bdcd5ab 100644 --- a/src/node/ext/call_credentials.cc +++ b/src/node/ext/call_credentials.cc @@ -211,6 +211,7 @@ NAN_METHOD(PluginCallback) { Utf8String details_utf8_str(info[1]); char *details = *details_utf8_str; grpc_metadata_array array; + grpc_metadata_array_init(&array); Local callback_data = Nan::To(info[3]).ToLocalChecked(); if (!CreateMetadataArray(Nan::To(info[2]).ToLocalChecked(), &array)){ @@ -226,6 +227,7 @@ NAN_METHOD(PluginCallback) { Nan::New("user_data").ToLocalChecked() ).ToLocalChecked().As()->Value(); cb(user_data, array.metadata, array.count, code, details); + DestroyMetadataArray(&array); } NAUV_WORK_CB(SendPluginCallback) {