Fixes error handling, leaks in secure sockets.

runtime/bin/secure_socket.cc

Index: runtime/bin/secure_socket.cc
diff --git a/runtime/bin/secure_socket.cc b/runtime/bin/secure_socket.cc
index f63d9d06f5aecd43b712c741f16e1df9113ab6e5..7d79dc60158e9d7b056afdf79b11bd59ea14812d 100644
--- a/runtime/bin/secure_socket.cc
+++ b/runtime/bin/secure_socket.cc
@@ -29,6 +29,15 @@
 
 #include "include/dart_api.h"
 
+// Return the error from the containing function if handle is an error handle.
+#define RETURN_IF_ERROR(handle)                                                \
+  {                                                                            \
+    Dart_Handle __handle = handle;                                             \
+    if (Dart_IsError((__handle))) {                                            \
+      return __handle;                                                         \
+    }                                                                          \
+  }
+
 namespace dart {
 namespace bin {
 
@@ -103,13 +112,30 @@ static SSLFilter* GetFilter(Dart_NativeArguments args) {
 }
 
 
-static void SetFilter(Dart_NativeArguments args, SSLFilter* filter) {
-  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
+static void DeleteFilter(
+    void* isolate_data,
+    Dart_WeakPersistentHandle handle,
+    void* context_pointer) {
+  SSLFilter* filter = reinterpret_cast<SSLFilter*>(context_pointer);
+  delete filter;
+}
+
+
+static Dart_Handle SetFilter(Dart_NativeArguments args, SSLFilter* filter) {
+  ASSERT(filter != NULL);
+  Dart_Handle dart_this = Dart_GetNativeArgument(args, 0);
+  RETURN_IF_ERROR(dart_this);
   ASSERT(Dart_IsInstance(dart_this));
-  ThrowIfError(Dart_SetNativeInstanceField(
+  Dart_Handle err = Dart_SetNativeInstanceField(
       dart_this,
       kSSLFilterNativeFieldIndex,
-      reinterpret_cast<intptr_t>(filter)));
+      reinterpret_cast<intptr_t>(filter));
+  RETURN_IF_ERROR(err);
+  Dart_NewWeakPersistentHandle(dart_this,
+                               reinterpret_cast<void*>(filter),
+                               sizeof(*filter),
+                               DeleteFilter);
+  return Dart_Null();
 }
 
 
@@ -134,19 +160,22 @@ static void FreeSecurityContext(
 }
 
 
-static void SetSecurityContext(Dart_NativeArguments args,
-                               SSL_CTX* context) {
+static Dart_Handle SetSecurityContext(Dart_NativeArguments args,
+                                      SSL_CTX* context) {
   const int approximate_size_of_context = 1500;
-  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  Dart_Handle dart_this = Dart_GetNativeArgument(args, 0);
+  RETURN_IF_ERROR(dart_this);
   ASSERT(Dart_IsInstance(dart_this));
-  ThrowIfError(Dart_SetNativeInstanceField(
+  Dart_Handle err = Dart_SetNativeInstanceField(
       dart_this,
       kSecurityContextNativeFieldIndex,
-      reinterpret_cast<intptr_t>(context)));
+      reinterpret_cast<intptr_t>(context));
+  RETURN_IF_ERROR(err);
   Dart_NewWeakPersistentHandle(dart_this,
                                context,
                                approximate_size_of_context,
                                FreeSecurityContext);
+  return Dart_Null();
 }
 
 
@@ -171,9 +200,19 @@ static void SetAlpnProtocolList(Dart_Handle protocols_handle,
 
 void FUNCTION_NAME(SecureSocket_Init)(Dart_NativeArguments args) {
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
-  SSLFilter* filter = new SSLFilter;
-  SetFilter(args, filter);
-  filter->Init(dart_this);
+  SSLFilter* filter = new SSLFilter();
+  Dart_Handle err = SetFilter(args, filter);
+  if (Dart_IsError(err)) {
+    delete filter;
+    Dart_PropagateError(err);
+  }
+  err = filter->Init(dart_this);
+  if (Dart_IsError(err)) {
+    // The finalizer was set up by SetFilter. It will delete `filter` if there
+    // is an error.
+    filter->Destroy();
+    Dart_PropagateError(err);
+  }
 }
 
 
@@ -215,9 +254,13 @@ void FUNCTION_NAME(SecureSocket_Connect)(Dart_NativeArguments args) {
 
 void FUNCTION_NAME(SecureSocket_Destroy)(Dart_NativeArguments args) {
   SSLFilter* filter = GetFilter(args);
-  SetFilter(args, NULL);
+  // The SSLFilter is deleted in the finalizer for the Dart object created by
+  // SetFilter. There is no need to NULL-out the native field for the SSLFilter
+  // here because the SSLFilter won't be deleted until the finalizer for the
+  // Dart object runs while the Dart object is being GCd. This approach avoids a
+  // leak if Destroy isn't called, and avoids a NULL-dereference if Destroy is
+  // called more than once.

[Ivan Posva, 2016/03/02 06:17:21]

Thanks!

   filter->Destroy();
-  delete filter;
 }
 
 
@@ -271,7 +314,8 @@ void FUNCTION_NAME(SecureSocket_RegisterBadCertificateCallback)(
 
 void FUNCTION_NAME(SecureSocket_PeerCertificate)
     (Dart_NativeArguments args) {
-  Dart_SetReturnValue(args, GetFilter(args)->PeerCertificate());
+  Dart_Handle cert = ThrowIfError(GetFilter(args)->PeerCertificate());
+  Dart_SetReturnValue(args, cert);
 }
 
 
@@ -281,19 +325,34 @@ void FUNCTION_NAME(SecureSocket_FilterPointer)(Dart_NativeArguments args) {
 }
 
 
+static void ReleaseCertificate(
+    void* isolate_data,
+    Dart_WeakPersistentHandle handle,
+    void* context_pointer) {
+  X509* cert = reinterpret_cast<X509*>(context_pointer);
+  X509_free(cert);
+}
+
+
+// Returns the handle for a Dart object wrapping the X509 certificate object.
+// The caller should own a reference to the X509 object whose reference count
+// won't drop to zero before the ReleaseCertificate finalizer runs.
 static Dart_Handle WrappedX509Certificate(X509* certificate) {
+  const intptr_t approximate_size_of_certificate = 1500;
   if (certificate == NULL) {
     return Dart_Null();
   }
   Dart_Handle x509_type =
       DartUtils::GetDartType(DartUtils::kIOLibURL, "X509Certificate");
   if (Dart_IsError(x509_type)) {
+    X509_free(certificate);
     return x509_type;
   }
   Dart_Handle arguments[] = { NULL };
   Dart_Handle result =
       Dart_New(x509_type, DartUtils::NewString("_"), 0, arguments);
   if (Dart_IsError(result)) {
+    X509_free(certificate);
     return result;
   }
   ASSERT(Dart_IsInstance(result));
@@ -302,8 +361,13 @@ static Dart_Handle WrappedX509Certificate(X509* certificate) {
       kX509NativeFieldIndex,
       reinterpret_cast<intptr_t>(certificate));
   if (Dart_IsError(status)) {
+    X509_free(certificate);
     return status;
   }
+  Dart_NewWeakPersistentHandle(result,
+                               reinterpret_cast<void*>(certificate),
+                               approximate_size_of_certificate,
+                               ReleaseCertificate);
   return result;
 }
 
@@ -326,6 +390,11 @@ int CertificateCallback(int preverify_ok, X509_STORE_CTX* store_ctx) {
   if (Dart_IsNull(callback)) {
     return 0;
   }
+
+  // Upref since the Dart X509 object may outlive the SecurityContext.
+  if (certificate != NULL) {
+    X509_up_ref(certificate);
+  }
   Dart_Handle args[1];
   args[0] = WrappedX509Certificate(certificate);
   if (Dart_IsError(args[0])) {
@@ -354,7 +423,11 @@ void FUNCTION_NAME(SecurityContext_Allocate)(Dart_NativeArguments args) {
   SSL_CTX_set_min_version(context, TLS1_VERSION);
   SSL_CTX_set_cipher_list(context, "HIGH:MEDIUM");
   SSL_CTX_set_cipher_list_tls11(context, "HIGH:MEDIUM");
-  SetSecurityContext(args, context);
+  Dart_Handle err = SetSecurityContext(args, context);
+  if (Dart_IsError(err)) {
+    SSL_CTX_free(context);
+    Dart_PropagateError(err);
+  }
 }
 
 
@@ -1154,7 +1227,7 @@ bool SSLFilter::ProcessAllBuffers(int starts[kNumBuffers],
 }
 
 
-void SSLFilter::Init(Dart_Handle dart_this) {
+Dart_Handle SSLFilter::Init(Dart_Handle dart_this) {
   if (!library_initialized_) {
     InitializeLibrary();
   }
@@ -1168,24 +1241,40 @@ void SSLFilter::Init(Dart_Handle dart_this) {
   bad_certificate_callback_ = Dart_NewPersistentHandle(Dart_Null());
   ASSERT(bad_certificate_callback_ != NULL);
 
-  InitializeBuffers(dart_this);
+  // Caller handles cleanup on an error.
+  return InitializeBuffers(dart_this);
 }
 
 
-void SSLFilter::InitializeBuffers(Dart_Handle dart_this) {
+Dart_Handle SSLFilter::InitializeBuffers(Dart_Handle dart_this) {
   // Create SSLFilter buffers as ExternalUint8Array objects.
-  Dart_Handle dart_buffers_object = ThrowIfError(
-      Dart_GetField(dart_this, DartUtils::NewString("buffers")));
-  Dart_Handle secure_filter_impl_type =
-      Dart_InstanceGetType(dart_this);
-  Dart_Handle dart_buffer_size = ThrowIfError(
-      Dart_GetField(secure_filter_impl_type, DartUtils::NewString("SIZE")));
-  int64_t buffer_size = DartUtils::GetIntegerValue(dart_buffer_size);
-  Dart_Handle dart_encrypted_buffer_size = ThrowIfError(
-      Dart_GetField(secure_filter_impl_type,
-                    DartUtils::NewString("ENCRYPTED_SIZE")));
-  int64_t encrypted_buffer_size =
-      DartUtils::GetIntegerValue(dart_encrypted_buffer_size);
+  Dart_Handle buffers_string = DartUtils::NewString("buffers");
+  RETURN_IF_ERROR(buffers_string);
+  Dart_Handle dart_buffers_object = Dart_GetField(dart_this, buffers_string);
+  RETURN_IF_ERROR(dart_buffers_object);
+  Dart_Handle secure_filter_impl_type = Dart_InstanceGetType(dart_this);
+  RETURN_IF_ERROR(secure_filter_impl_type);
+  Dart_Handle size_string = DartUtils::NewString("SIZE");
+  RETURN_IF_ERROR(size_string);
+  Dart_Handle dart_buffer_size = Dart_GetField(
+      secure_filter_impl_type, size_string);
+  RETURN_IF_ERROR(dart_buffer_size);
+
+  int64_t buffer_size = 0;
+  Dart_Handle err = Dart_IntegerToInt64(dart_buffer_size, &buffer_size);
+  RETURN_IF_ERROR(err);
+
+  Dart_Handle encrypted_size_string = DartUtils::NewString("ENCRYPTED_SIZE");
+  RETURN_IF_ERROR(encrypted_size_string);
+
+  Dart_Handle dart_encrypted_buffer_size = Dart_GetField(
+      secure_filter_impl_type, encrypted_size_string);
+  RETURN_IF_ERROR(dart_encrypted_buffer_size);
+
+  int64_t encrypted_buffer_size = 0;
+  err = Dart_IntegerToInt64(dart_encrypted_buffer_size, &encrypted_buffer_size);
+  RETURN_IF_ERROR(err);
+
   if (buffer_size <= 0 || buffer_size > 1 * MB) {
     FATAL("Invalid buffer size in _ExternalBuffer");
   }
@@ -1195,21 +1284,44 @@ void SSLFilter::InitializeBuffers(Dart_Handle dart_this) {
   buffer_size_ = static_cast<int>(buffer_size);
   encrypted_buffer_size_ = static_cast<int>(encrypted_buffer_size);
 
-
   Dart_Handle data_identifier = DartUtils::NewString("data");
+  RETURN_IF_ERROR(data_identifier);
+
+  for (int i = 0; i < kNumBuffers; i++) {
+    int size = isBufferEncrypted(i) ? encrypted_buffer_size_ : buffer_size_;
+    buffers_[i] = new uint8_t[size];
+    ASSERT(buffers_[i] != NULL);
+    dart_buffer_objects_[i] = NULL;
+  }
+
+  Dart_Handle result = Dart_Null();
   for (int i = 0; i < kNumBuffers; ++i) {
     int size = isBufferEncrypted(i) ? encrypted_buffer_size_ : buffer_size_;
-    dart_buffer_objects_[i] =
-        Dart_NewPersistentHandle(Dart_ListGetAt(dart_buffers_object, i));
+    result = Dart_ListGetAt(dart_buffers_object, i);
+    if (Dart_IsError(result)) {
+      break;
+    }
+
+    dart_buffer_objects_[i] = Dart_NewPersistentHandle(result);
     ASSERT(dart_buffer_objects_[i] != NULL);
-    buffers_[i] = new uint8_t[size];
-    Dart_Handle data = ThrowIfError(
-        Dart_NewExternalTypedData(Dart_TypedData_kUint8, buffers_[i], size));
-    ThrowIfError(
-        Dart_SetField(Dart_HandleFromPersistent(dart_buffer_objects_[i]),
-                      data_identifier,
-                      data));
+    Dart_Handle data =
+        Dart_NewExternalTypedData(Dart_TypedData_kUint8, buffers_[i], size);
+    if (Dart_IsError(data)) {
+      result = data;
+      break;
+    }
+    result = Dart_HandleFromPersistent(dart_buffer_objects_[i]);
+    if (Dart_IsError(result)) {
+      break;
+    }
+    result = Dart_SetField(result, data_identifier, data);
+    if (Dart_IsError(result)) {
+      break;
+    }
   }
+
+  // Caller handles cleanup on an error.
+  return result;
 }
 
 
@@ -1241,12 +1353,10 @@ void SSLFilter::InitializeLibrary() {
 
 
 Dart_Handle SSLFilter::PeerCertificate() {
+  // SSL_get_peer_certificate incs the refcount of certificate. X509_free is
+  // called by the finalizer set up by WrappedX509Certificate.
   X509* certificate = SSL_get_peer_certificate(ssl_);
-  Dart_Handle x509_object = WrappedX509Certificate(certificate);
-  if (Dart_IsError(x509_object)) {
-    Dart_PropagateError(x509_object);
-  }
-  return x509_object;
+  return WrappedX509Certificate(certificate);
 }
 
 
@@ -1484,7 +1594,7 @@ void SSLFilter::Renegotiate(bool use_session_cache,
 }
 
 
-void SSLFilter::Destroy() {
+SSLFilter::~SSLFilter() {
   if (ssl_ != NULL) {
     SSL_free(ssl_);
     ssl_ = NULL;
@@ -1498,13 +1608,37 @@ void SSLFilter::Destroy() {
     hostname_ = NULL;
   }
   for (int i = 0; i < kNumBuffers; ++i) {
-    Dart_DeletePersistentHandle(dart_buffer_objects_[i]);
-    delete[] buffers_[i];
+    if (buffers_[i] != NULL) {
+      delete[] buffers_[i];
+      buffers_[i] = NULL;
+    }
+  }
+}
+
+
+void SSLFilter::Destroy() {
+  for (int i = 0; i < kNumBuffers; ++i) {
+    if (dart_buffer_objects_[i] != NULL) {
+      Dart_DeletePersistentHandle(dart_buffer_objects_[i]);
+      dart_buffer_objects_[i] = NULL;
+    }
+  }
+  if (string_start_ != NULL) {
+    Dart_DeletePersistentHandle(string_start_);
+    string_start_ = NULL;
+  }
+  if (string_length_ != NULL) {
+    Dart_DeletePersistentHandle(string_length_);
+    string_length_ = NULL;
+  }
+  if (handshake_complete_ != NULL) {
+    Dart_DeletePersistentHandle(handshake_complete_);
+    handshake_complete_ = NULL;
+  }
+  if (bad_certificate_callback_ != NULL) {
+    Dart_DeletePersistentHandle(bad_certificate_callback_);
+    bad_certificate_callback_ = NULL;
   }
-  Dart_DeletePersistentHandle(string_start_);
-  Dart_DeletePersistentHandle(string_length_);
-  Dart_DeletePersistentHandle(handshake_complete_);
-  Dart_DeletePersistentHandle(bad_certificate_callback_);
 }