Fix object lifetime and memory management problems in server code

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;
+  StringOrNull root_certs;
-  grpc_ssl_pem_key_cert_pair key_cert_pair = {NULL, NULL};
+  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) !=
+  grpc_ssl_pem_key_cert_pair key_cert_pair = {private_key.get(),
-      (key_cert_pair.cert_chain == NULL)) {
+                                              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();

packages/grpc-native-core/ext/server.cc

@@ -62,28 +62,31 @@ using v8::Value;
 Nan::Callback *Server::constructor;
 Persistent<FunctionTemplate> Server::fun_tpl;
 
-static Callback *shutdown_callback = NULL;
+static Persistent<Function> shutdown_cb;
 
 class ServerShutdownOp : public Op {
  public:
-  ServerShutdownOp(grpc_server *server) : server(server) {}
+  ServerShutdownOp(Server *server) : server(server) {}
 
   ~ServerShutdownOp() {}
 
-  Local<Value> GetNodeValue() const { return Nan::Null(); }
+  Local<Value> GetNodeValue() const {
+    EscapableHandleScope scope;
+    return scope.Escape(server->handle());
+  }
 
   bool ParseOp(Local<Value> 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
+    if (success) {
-       completes successfully */
+      server->FinishShutdown();
-    grpc_server_destroy(server);
+    }
   }
 
-  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 {
+NAN_METHOD(ShutdownCallback) {
- public:
+  HandleScope scope;
-  TryShutdownOp(Server *server, Local<Value> server_value) : server(server) {
+  if (!info[0]->IsNull()) {
-    server_persist.Reset(server_value);
+    return Nan::ThrowError("forceShutdown failed somehow");
-  }
-  Local<Value> GetNodeValue() const {
-    EscapableHandleScope scope;
-    return scope.Escape(Nan::New(server_persist));
-  }
-  bool ParseOp(Local<Value> value, grpc_op *out) { return true; }
-  bool IsFinalOp() { return false; }
-  void OnComplete(bool success) {
-    if (success) {
-      server->DestroyWrappedServer();
-    }
   }
+}
 
- protected:
+Server::Server(grpc_server *server) :
-  std::string GetTypeString() const { return "try_shutdown"; }
+    wrapped_server(server), is_shutdown(false) {}
 
- private:
+Server::~Server() { grpc_server_destroy(this->wrapped_server); }
-  Server *server;
-  Nan::Persistent<v8::Value, Nan::CopyablePersistentTraits<v8::Value>>
-      server_persist;
-};
-
-Server::Server(grpc_server *server) : wrapped_server(server) {}
-
-Server::~Server() { this->ShutdownServer(); }
 
 void Server::Init(Local<Object> exports) {
   HandleScope scope;
@@ -175,6 +160,9 @@ void Server::Init(Local<Object> exports) {
   Local<Function> ctr = Nan::GetFunction(tpl).ToLocalChecked();
   Nan::Set(exports, Nan::New("Server").ToLocalChecked(), ctr);
   constructor = new Callback(ctr);
+  Local<FunctionTemplate> shutdown_tpl =
+      Nan::New<FunctionTemplate>(ShutdownCallback);
+  shutdown_cb.Reset(Nan::GetFunction(shutdown_tpl).ToLocalChecked());
 }
 
 bool Server::HasInstance(Local<Value> val) {
@@ -182,40 +170,25 @@ bool Server::HasInstance(Local<Value> val) {
   return Nan::New(fun_tpl)->HasInstance(val);
 }
 
-void Server::DestroyWrappedServer() {
+void Server::FinishShutdown() {
-  if (this->wrapped_server != NULL) {
+  is_shutdown = true;
-    grpc_server_destroy(this->wrapped_server);
+  running_self_ref.Reset();
-    this->wrapped_server = NULL;
-  }
-}
-
-NAN_METHOD(ServerShutdownCallback) {
-  if (!info[0]->IsNull()) {
-    return Nan::ThrowError("forceShutdown failed somehow");
-  }
 }
 
 void Server::ShutdownServer() {
   Nan::HandleScope scope;
-  if (this->wrapped_server != NULL) {
+  if (!this->is_shutdown) {
-    if (shutdown_callback == NULL) {
+    Callback *shutdown_callback =
-      Local<FunctionTemplate> callback_tpl =
+        new Callback(Nan::New(shutdown_cb));
-          Nan::New<FunctionTemplate>(ServerShutdownCallback);
+    ServerShutdownOp *op = new ServerShutdownOp(this);
-      shutdown_callback =
-          new Callback(Nan::GetFunction(callback_tpl).ToLocalChecked());
-    }
-
-    ServerShutdownOp *op = new ServerShutdownOp(this->wrapped_server);
     unique_ptr<OpVec> ops(new OpVec());
     ops->push_back(unique_ptr<Op>(op));
 
     grpc_server_shutdown_and_notify(
         this->wrapped_server, GetCompletionQueue(),
-        new struct tag(new Callback(**shutdown_callback), ops.release(), NULL,
+        new struct tag(shutdown_callback, ops.release(), NULL, Nan::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<Server>(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<Server>(info.This());
-  if (server->wrapped_server == NULL) {
+  ServerShutdownOp *op = new ServerShutdownOp(server);
-    // Server is already shut down. Call callback immediately.
-    Nan::Callback callback(info[0].As<Function>());
-    callback.Call(0, {});
-    return;
-  }
-  TryShutdownOp *op = new TryShutdownOp(server, info.This());
   unique_ptr<OpVec> ops(new OpVec());
   ops->push_back(unique_ptr<Op>(op));
   grpc_server_shutdown_and_notify(

packages/grpc-native-core/ext/server.h

@@ -38,7 +38,7 @@ class Server : public Nan::ObjectWrap {
      JavaScript constructor */
   static bool HasInstance(v8::Local<v8::Value> 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<v8::FunctionTemplate> fun_tpl;
+  Nan::Persistent<v8::Value> running_self_ref;
 
   grpc_server *wrapped_server;
   grpc_completion_queue *shutdown_queue;
+  bool is_shutdown;
 };
 
 }  // namespace node

packages/grpc-native-core/ext/server_credentials.cc

@@ -16,12 +16,14 @@
  *
  */
 
+#include <nan.h>
 #include <node.h>
 
 #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<Array> pair_list = Local<Array>::Cast(info[1]);
   uint32_t key_cert_pair_count = pair_list->Length();
-  grpc_ssl_pem_key_cert_pair *key_cert_pairs =
+  grpc_ssl_pem_key_cert_pair key_cert_pairs[key_cert_pair_count];
-      new grpc_ssl_pem_key_cert_pair[key_cert_pair_count];
+  StringOrNull key_strings[key_cert_pair_count];
-
+  StringOrNull cert_strings[key_cert_pair_count];
   Local<String> key_key = Nan::New("private_key").ToLocalChecked();
   Local<String> cert_key = Nan::New("cert_chain").ToLocalChecked();
 
   for (uint32_t i = 0; i < key_cert_pair_count; i++) {
     Local<Value> 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<Object> pair_obj = Nan::To<Object>(pair_val).ToLocalChecked();
     Local<Value> maybe_key = Nan::Get(pair_obj, key_key).ToLocalChecked();
     Local<Value> 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_strings[i].assign(maybe_key);
-    key_cert_pairs[i].cert_chain = ::node::Buffer::Data(maybe_cert);
+    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 {

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 <node.h>
+#include <nan.h>
+
+#include <string>
+
+namespace grpc {
+namespace node {
+
+class StringOrNull {
+ public:
+  StringOrNull() : assigned(false) { }
+  void assign(v8::Local<v8::Value> 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_