From 0210f160ed78fd0419e8a3e3263d8ebc8097ad08 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 2 Apr 2018 14:35:46 -0700 Subject: [PATCH] Fix object lifetime and memory management problems in server code --- .../ext/channel_credentials.cc | 19 ++-- packages/grpc-native-core/ext/server.cc | 96 +++++++------------ packages/grpc-native-core/ext/server.h | 4 +- .../ext/server_credentials.cc | 24 ++--- packages/grpc-native-core/ext/util.h | 52 ++++++++++ 5 files changed, 110 insertions(+), 85 deletions(-) create mode 100644 packages/grpc-native-core/ext/util.h diff --git a/packages/grpc-native-core/ext/channel_credentials.cc b/packages/grpc-native-core/ext/channel_credentials.cc index 9e87fb61..da67f35a 100644 --- a/packages/grpc-native-core/ext/channel_credentials.cc +++ b/packages/grpc-native-core/ext/channel_credentials.cc @@ -21,6 +21,7 @@ #include "call.h" #include "call_credentials.h" #include "channel_credentials.h" +#include "util.h" #include "grpc/grpc.h" #include "grpc/grpc_security.h" #include "grpc/support/log.h" @@ -120,33 +121,35 @@ NAN_METHOD(ChannelCredentials::New) { } NAN_METHOD(ChannelCredentials::CreateSsl) { - char *root_certs = NULL; - grpc_ssl_pem_key_cert_pair key_cert_pair = {NULL, NULL}; + StringOrNull root_certs; + StringOrNull private_key; + StringOrNull cert_chain; if (::node::Buffer::HasInstance(info[0])) { - root_certs = ::node::Buffer::Data(info[0]); + root_certs.assign(info[0]); } else if (!(info[0]->IsNull() || info[0]->IsUndefined())) { return Nan::ThrowTypeError("createSsl's first argument must be a Buffer"); } if (::node::Buffer::HasInstance(info[1])) { - key_cert_pair.private_key = ::node::Buffer::Data(info[1]); + private_key.assign(info[1]); } else if (!(info[1]->IsNull() || info[1]->IsUndefined())) { return Nan::ThrowTypeError( "createSSl's second argument must be a Buffer if provided"); } if (::node::Buffer::HasInstance(info[2])) { - key_cert_pair.cert_chain = ::node::Buffer::Data(info[2]); + cert_chain.assign(info[2]); } else if (!(info[2]->IsNull() || info[2]->IsUndefined())) { return Nan::ThrowTypeError( "createSSl's third argument must be a Buffer if provided"); } - if ((key_cert_pair.private_key == NULL) != - (key_cert_pair.cert_chain == NULL)) { + grpc_ssl_pem_key_cert_pair key_cert_pair = {private_key.get(), + cert_chain.get()}; + if (private_key.isAssigned() != cert_chain.isAssigned()) { return Nan::ThrowError( "createSsl's second and third arguments must be" " provided or omitted together"); } grpc_channel_credentials *creds = grpc_ssl_credentials_create( - root_certs, key_cert_pair.private_key == NULL ? NULL : &key_cert_pair, + root_certs.get(), private_key.isAssigned() ? &key_cert_pair : NULL, NULL); if (creds == NULL) { info.GetReturnValue().SetNull(); diff --git a/packages/grpc-native-core/ext/server.cc b/packages/grpc-native-core/ext/server.cc index d5b4763b..aa56b5e5 100644 --- a/packages/grpc-native-core/ext/server.cc +++ b/packages/grpc-native-core/ext/server.cc @@ -62,28 +62,31 @@ using v8::Value; Nan::Callback *Server::constructor; Persistent Server::fun_tpl; -static Callback *shutdown_callback = NULL; +static Persistent shutdown_cb; class ServerShutdownOp : public Op { public: - ServerShutdownOp(grpc_server *server) : server(server) {} + ServerShutdownOp(Server *server) : server(server) {} ~ServerShutdownOp() {} - Local GetNodeValue() const { return Nan::Null(); } + Local GetNodeValue() const { + EscapableHandleScope scope; + return scope.Escape(server->handle()); + } bool ParseOp(Local value, grpc_op *out) { return true; } bool IsFinalOp() { return false; } void OnComplete(bool success) { - /* Because cancel_all_calls was called, we assume that shutdown_and_notify - completes successfully */ - grpc_server_destroy(server); + if (success) { + server->FinishShutdown(); + } } - grpc_server *server; + Server *server; protected: - std::string GetTypeString() const { return "shutdown"; } + std::string GetTypeString() const { return "try_shutdown"; } }; class NewCallOp : public Op { @@ -131,35 +134,17 @@ class NewCallOp : public Op { std::string GetTypeString() const { return "new_call"; } }; -class TryShutdownOp : public Op { - public: - TryShutdownOp(Server *server, Local server_value) : server(server) { - server_persist.Reset(server_value); - } - Local GetNodeValue() const { - EscapableHandleScope scope; - return scope.Escape(Nan::New(server_persist)); - } - bool ParseOp(Local value, grpc_op *out) { return true; } - bool IsFinalOp() { return false; } - void OnComplete(bool success) { - if (success) { - server->DestroyWrappedServer(); - } +NAN_METHOD(ShutdownCallback) { + HandleScope scope; + if (!info[0]->IsNull()) { + return Nan::ThrowError("forceShutdown failed somehow"); } +} - protected: - std::string GetTypeString() const { return "try_shutdown"; } +Server::Server(grpc_server *server) : + wrapped_server(server), is_shutdown(false) {} - private: - Server *server; - Nan::Persistent> - server_persist; -}; - -Server::Server(grpc_server *server) : wrapped_server(server) {} - -Server::~Server() { this->ShutdownServer(); } +Server::~Server() { grpc_server_destroy(this->wrapped_server); } void Server::Init(Local exports) { HandleScope scope; @@ -175,6 +160,9 @@ void Server::Init(Local exports) { Local ctr = Nan::GetFunction(tpl).ToLocalChecked(); Nan::Set(exports, Nan::New("Server").ToLocalChecked(), ctr); constructor = new Callback(ctr); + Local shutdown_tpl = + Nan::New(ShutdownCallback); + shutdown_cb.Reset(Nan::GetFunction(shutdown_tpl).ToLocalChecked()); } bool Server::HasInstance(Local val) { @@ -182,40 +170,25 @@ bool Server::HasInstance(Local val) { return Nan::New(fun_tpl)->HasInstance(val); } -void Server::DestroyWrappedServer() { - if (this->wrapped_server != NULL) { - grpc_server_destroy(this->wrapped_server); - this->wrapped_server = NULL; - } -} - -NAN_METHOD(ServerShutdownCallback) { - if (!info[0]->IsNull()) { - return Nan::ThrowError("forceShutdown failed somehow"); - } +void Server::FinishShutdown() { + is_shutdown = true; + running_self_ref.Reset(); } void Server::ShutdownServer() { Nan::HandleScope scope; - if (this->wrapped_server != NULL) { - if (shutdown_callback == NULL) { - Local callback_tpl = - Nan::New(ServerShutdownCallback); - shutdown_callback = - new Callback(Nan::GetFunction(callback_tpl).ToLocalChecked()); - } - - ServerShutdownOp *op = new ServerShutdownOp(this->wrapped_server); + if (!this->is_shutdown) { + Callback *shutdown_callback = + new Callback(Nan::New(shutdown_cb)); + ServerShutdownOp *op = new ServerShutdownOp(this); unique_ptr ops(new OpVec()); ops->push_back(unique_ptr(op)); grpc_server_shutdown_and_notify( this->wrapped_server, GetCompletionQueue(), - new struct tag(new Callback(**shutdown_callback), ops.release(), NULL, - Nan::Null())); + new struct tag(shutdown_callback, ops.release(), NULL, Nan::Null())); grpc_server_cancel_all_calls(this->wrapped_server); CompletionQueueNext(); - this->wrapped_server = NULL; } } @@ -304,6 +277,7 @@ NAN_METHOD(Server::Start) { return Nan::ThrowTypeError("start can only be called on a Server"); } Server *server = ObjectWrap::Unwrap(info.This()); + server->running_self_ref.Reset(info.This()); grpc_server_start(server->wrapped_server); } @@ -316,13 +290,7 @@ NAN_METHOD(Server::TryShutdown) { return Nan::ThrowError("tryShutdown's argument must be a callback"); } Server *server = ObjectWrap::Unwrap(info.This()); - if (server->wrapped_server == NULL) { - // Server is already shut down. Call callback immediately. - Nan::Callback callback(info[0].As()); - callback.Call(0, {}); - return; - } - TryShutdownOp *op = new TryShutdownOp(server, info.This()); + ServerShutdownOp *op = new ServerShutdownOp(server); unique_ptr ops(new OpVec()); ops->push_back(unique_ptr(op)); grpc_server_shutdown_and_notify( diff --git a/packages/grpc-native-core/ext/server.h b/packages/grpc-native-core/ext/server.h index 66b3ac52..188e7054 100644 --- a/packages/grpc-native-core/ext/server.h +++ b/packages/grpc-native-core/ext/server.h @@ -38,7 +38,7 @@ class Server : public Nan::ObjectWrap { JavaScript constructor */ static bool HasInstance(v8::Local val); - void DestroyWrappedServer(); + void FinishShutdown(); private: explicit Server(grpc_server *server); @@ -58,9 +58,11 @@ class Server : public Nan::ObjectWrap { static NAN_METHOD(ForceShutdown); static Nan::Callback *constructor; static Nan::Persistent fun_tpl; + Nan::Persistent running_self_ref; grpc_server *wrapped_server; grpc_completion_queue *shutdown_queue; + bool is_shutdown; }; } // namespace node diff --git a/packages/grpc-native-core/ext/server_credentials.cc b/packages/grpc-native-core/ext/server_credentials.cc index b7fa4788..7172a165 100644 --- a/packages/grpc-native-core/ext/server_credentials.cc +++ b/packages/grpc-native-core/ext/server_credentials.cc @@ -16,12 +16,14 @@ * */ +#include #include #include "grpc/grpc.h" #include "grpc/grpc_security.h" #include "grpc/support/log.h" #include "server_credentials.h" +#include "util.h" namespace grpc { namespace node { @@ -119,9 +121,9 @@ NAN_METHOD(ServerCredentials::New) { NAN_METHOD(ServerCredentials::CreateSsl) { Nan::HandleScope scope; - char *root_certs = NULL; + StringOrNull root_certs; if (::node::Buffer::HasInstance(info[0])) { - root_certs = ::node::Buffer::Data(info[0]); + root_certs.assign(info[0]); } else if (!(info[0]->IsNull() || info[0]->IsUndefined())) { return Nan::ThrowTypeError( "createSSl's first argument must be a Buffer if provided"); @@ -145,36 +147,34 @@ NAN_METHOD(ServerCredentials::CreateSsl) { } Local pair_list = Local::Cast(info[1]); uint32_t key_cert_pair_count = pair_list->Length(); - grpc_ssl_pem_key_cert_pair *key_cert_pairs = - new grpc_ssl_pem_key_cert_pair[key_cert_pair_count]; - + grpc_ssl_pem_key_cert_pair key_cert_pairs[key_cert_pair_count]; + StringOrNull key_strings[key_cert_pair_count]; + StringOrNull cert_strings[key_cert_pair_count]; Local key_key = Nan::New("private_key").ToLocalChecked(); Local cert_key = Nan::New("cert_chain").ToLocalChecked(); for (uint32_t i = 0; i < key_cert_pair_count; i++) { Local pair_val = Nan::Get(pair_list, i).ToLocalChecked(); if (!pair_val->IsObject()) { - delete[] key_cert_pairs; return Nan::ThrowTypeError("Key/cert pairs must be objects"); } Local pair_obj = Nan::To(pair_val).ToLocalChecked(); Local maybe_key = Nan::Get(pair_obj, key_key).ToLocalChecked(); Local maybe_cert = Nan::Get(pair_obj, cert_key).ToLocalChecked(); if (!::node::Buffer::HasInstance(maybe_key)) { - delete[] key_cert_pairs; return Nan::ThrowTypeError("private_key must be a Buffer"); } if (!::node::Buffer::HasInstance(maybe_cert)) { - delete[] key_cert_pairs; return Nan::ThrowTypeError("cert_chain must be a Buffer"); } - key_cert_pairs[i].private_key = ::node::Buffer::Data(maybe_key); - key_cert_pairs[i].cert_chain = ::node::Buffer::Data(maybe_cert); + key_strings[i].assign(maybe_key); + cert_strings[i].assign(maybe_cert); + key_cert_pairs[i].private_key = key_strings[i].get(); + key_cert_pairs[i].cert_chain = cert_strings[i].get(); } grpc_server_credentials *creds = grpc_ssl_server_credentials_create_ex( - root_certs, key_cert_pairs, key_cert_pair_count, + root_certs.get(), key_cert_pairs, key_cert_pair_count, client_certificate_request, NULL); - delete[] key_cert_pairs; if (creds == NULL) { info.GetReturnValue().SetNull(); } else { diff --git a/packages/grpc-native-core/ext/util.h b/packages/grpc-native-core/ext/util.h new file mode 100644 index 00000000..c481922a --- /dev/null +++ b/packages/grpc-native-core/ext/util.h @@ -0,0 +1,52 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef NET_GRPC_NODE_UTIL_H_ +#define NET_GRPC_NODE_UTIL_H_ + +#include +#include + +#include + +namespace grpc { +namespace node { + +class StringOrNull { + public: + StringOrNull() : assigned(false) { } + void assign(v8::Local buffer) { + str_ = std::string(::node::Buffer::Data(buffer), + ::node::Buffer::Length(buffer)); + assigned = true; + } + const char * get() { + return assigned ? str_.c_str() : NULL; + } + bool isAssigned() { + return assigned; + } + private: + std::string str_; + bool assigned; +}; + +} // namespace node +} // namespace grpc + +#endif // NET_GRPC_NODE_UTIL_H_