Fixes error handling, leaks in secure sockets.

runtime/bin/secure_socket.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "bin/secure_socket.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <stdio.h>
@@ skipping 11 matching lines @@
 #include "bin/dartutils.h"
 #include "bin/lockers.h"
 #include "bin/log.h"
 #include "bin/socket.h"
 #include "bin/thread.h"
 #include "bin/utils.h"
 #include "platform/utils.h"
 
 #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 {
 
 bool SSLFilter::library_initialized_ = false;
 // To protect library initialization.
 Mutex* SSLFilter::mutex_ = new Mutex();
 int SSLFilter::filter_ssl_index;
 
 static const int kSSLFilterNativeFieldIndex = 0;
 static const int kSecurityContextNativeFieldIndex = 0;
@@ skipping 54 matching lines @@
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   ASSERT(Dart_IsInstance(dart_this));
   ThrowIfError(Dart_GetNativeInstanceField(
       dart_this,
       kSSLFilterNativeFieldIndex,
       reinterpret_cast<intptr_t*>(&filter)));
   return filter;
 }
 
 
-static void SetFilter(Dart_NativeArguments args, SSLFilter* filter) {
-  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
-  ASSERT(Dart_IsInstance(dart_this));
-  ThrowIfError(Dart_SetNativeInstanceField(
-      dart_this,
-      kSSLFilterNativeFieldIndex,
-      reinterpret_cast<intptr_t>(filter)));
+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));
+  Dart_Handle err = Dart_SetNativeInstanceField(
+      dart_this,
+      kSSLFilterNativeFieldIndex,
+      reinterpret_cast<intptr_t>(filter));
+  RETURN_IF_ERROR(err);
+  Dart_NewWeakPersistentHandle(dart_this,
+                               reinterpret_cast<void*>(filter),
+                               sizeof(*filter),
+                               DeleteFilter);
+  return Dart_Null();
+}
+
+
 static SSL_CTX* GetSecurityContext(Dart_NativeArguments args) {
   SSL_CTX* context;
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   ASSERT(Dart_IsInstance(dart_this));
   ThrowIfError(Dart_GetNativeInstanceField(
       dart_this,
       kSecurityContextNativeFieldIndex,
       reinterpret_cast<intptr_t*>(&context)));
   return context;
 }
 
 
 static void FreeSecurityContext(
     void* isolate_data,
     Dart_WeakPersistentHandle handle,
     void* context_pointer) {
   SSL_CTX* context = static_cast<SSL_CTX*>(context_pointer);
   SSL_CTX_free(context);
 }
 
 
-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();
 }
 
 
 static X509* GetX509Certificate(Dart_NativeArguments args) {
   X509* certificate;
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   ASSERT(Dart_IsInstance(dart_this));
   ThrowIfError(Dart_GetNativeInstanceField(
       dart_this,
       kX509NativeFieldIndex,
       reinterpret_cast<intptr_t*>(&certificate)));
   return certificate;
 }
 
 
 // Forward declaration.
 static void SetAlpnProtocolList(Dart_Handle protocols_handle,
                                 SSL* ssl,
                                 SSL_CTX* context,
                                 bool is_server);
 
 
 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);
+  }
 }
 
 
 void FUNCTION_NAME(SecureSocket_Connect)(Dart_NativeArguments args) {
   Dart_Handle host_name_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
   Dart_Handle context_object = ThrowIfError(Dart_GetNativeArgument(args, 2));
   bool is_server = DartUtils::GetBooleanValue(Dart_GetNativeArgument(args, 3));
   bool request_client_certificate =
       DartUtils::GetBooleanValue(Dart_GetNativeArgument(args, 4));
   bool require_client_certificate =
@@ skipping 21 matching lines @@
                            context,
                            is_server,
                            request_client_certificate,
                            require_client_certificate,
                            protocols_handle);
 }
 
 
 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;
 }
 
 
 void FUNCTION_NAME(SecureSocket_Handshake)(Dart_NativeArguments args) {
   GetFilter(args)->Handshake();
 }
 
 
 void FUNCTION_NAME(SecureSocket_GetSelectedProtocol)(
     Dart_NativeArguments args) {
@@ skipping 33 matching lines @@
   if (!Dart_IsClosure(callback) && !Dart_IsNull(callback)) {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
         "Illegal argument to RegisterBadCertificateCallback"));
   }
   GetFilter(args)->RegisterBadCertificateCallback(callback);
 }
 
 
 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);
 }
 
 
 void FUNCTION_NAME(SecureSocket_FilterPointer)(Dart_NativeArguments args) {
   intptr_t filter_pointer = reinterpret_cast<intptr_t>(GetFilter(args));
   Dart_SetReturnValue(args, Dart_NewInteger(filter_pointer));
 }
 
 
+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));
   Dart_Handle status = Dart_SetNativeInstanceField(
       result,
       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;
 }
 
 
 int CertificateCallback(int preverify_ok, X509_STORE_CTX* store_ctx) {
   if (preverify_ok == 1) {
     return 1;
   }
   Dart_Isolate isolate = Dart_CurrentIsolate();
   if (isolate == NULL) {
     FATAL("CertificateCallback called with no current isolate\n");
   }
   X509* certificate = X509_STORE_CTX_get_current_cert(store_ctx);
   int ssl_index = SSL_get_ex_data_X509_STORE_CTX_idx();
   SSL* ssl = static_cast<SSL*>(
       X509_STORE_CTX_get_ex_data(store_ctx, ssl_index));
   SSLFilter* filter = static_cast<SSLFilter*>(
       SSL_get_ex_data(ssl, SSLFilter::filter_ssl_index));
   Dart_Handle callback = filter->bad_certificate_callback();
   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])) {
     filter->callback_error = args[0];
     return 0;
   }
   Dart_Handle result = Dart_InvokeClosure(callback, 1, args);
   if (!Dart_IsError(result) && !Dart_IsBoolean(result)) {
     result = Dart_NewUnhandledExceptionError(DartUtils::NewDartIOException(
         "HandshakeException",
         "BadCertificateCallback returned a value that was not a boolean",
         Dart_Null()));
   }
   if (Dart_IsError(result)) {
     filter->callback_error = result;
     return 0;
   }
   return DartUtils::GetBooleanValue(result);
 }
 
 
 void FUNCTION_NAME(SecurityContext_Allocate)(Dart_NativeArguments args) {
   SSLFilter::InitializeLibrary();
   SSL_CTX* context = SSL_CTX_new(TLS_method());
   SSL_CTX_set_verify(context, SSL_VERIFY_PEER, CertificateCallback);
   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);
+  }
 }
 
 
 int PasswordCallback(char* buf, int size, int rwflag, void* userdata) {
   char* password = static_cast<char*>(userdata);
   ASSERT(size == PEM_BUFSIZE);
   strncpy(buf, password, size);
   return strlen(password);
 }
 
@@ skipping 779 matching lines @@
         starts[i] = start;
         break;
       default:
         UNREACHABLE();
     }
   }
   return true;
 }
 
 
-void SSLFilter::Init(Dart_Handle dart_this) {
+Dart_Handle SSLFilter::Init(Dart_Handle dart_this) {
   if (!library_initialized_) {
     InitializeLibrary();
   }
   ASSERT(string_start_ == NULL);
   string_start_ = Dart_NewPersistentHandle(DartUtils::NewString("start"));
   ASSERT(string_start_ != NULL);
   ASSERT(string_length_ == NULL);
   string_length_ = Dart_NewPersistentHandle(DartUtils::NewString("length"));
   ASSERT(string_length_ != NULL);
   ASSERT(bad_certificate_callback_ == NULL);
   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");
   }
   if (encrypted_buffer_size <= 0 || encrypted_buffer_size > 1 * MB) {
     FATAL("Invalid encrypted buffer size in _ExternalBuffer");
   }
   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);
 
-  Dart_Handle data_identifier = DartUtils::NewString("data");
+  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;
 }
 
 
 void SSLFilter::RegisterHandshakeCompleteCallback(Dart_Handle complete) {
   ASSERT(NULL == handshake_complete_);
   handshake_complete_ = Dart_NewPersistentHandle(complete);
 
   ASSERT(handshake_complete_ != NULL);
 }
 
@@ skipping 11 matching lines @@
   if (!library_initialized_) {
     SSL_library_init();
     filter_ssl_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
     ASSERT(filter_ssl_index >= 0);
     library_initialized_ = true;
   }
 }
 
 
 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);
 }
 
 
 int AlpnCallback(SSL *ssl,
                  const uint8_t **out,
                  uint8_t *outlen,
                  const uint8_t *in,
                  unsigned int inlen,
                  void *arg) {
   // 'in' and 'arg' are sequences of (length, data) strings with 1-byte lengths.
@@ skipping 217 matching lines @@
                             bool require_client_certificate) {
   // The SSL_REQUIRE_CERTIFICATE option only takes effect if the
   // SSL_REQUEST_CERTIFICATE option is also set, so set it.
   request_client_certificate =
       request_client_certificate || require_client_certificate;
   // TODO(24070, 24069): Implement setting the client certificate parameters,
   //   and triggering rehandshake.
 }
 
 
-void SSLFilter::Destroy() {
+SSLFilter::~SSLFilter() {
   if (ssl_ != NULL) {
     SSL_free(ssl_);
     ssl_ = NULL;
   }
   if (socket_side_ != NULL) {
     BIO_free(socket_side_);
     socket_side_ = NULL;
   }
   if (hostname_ != NULL) {
     free(hostname_);
     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;
+    }
   }
-  Dart_DeletePersistentHandle(string_start_);
-  Dart_DeletePersistentHandle(string_length_);
-  Dart_DeletePersistentHandle(handshake_complete_);
-  Dart_DeletePersistentHandle(bad_certificate_callback_);
 }
 
 
+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;
+  }
+}
+
+
 /* Read decrypted data from the filter to the circular buffer */
 int SSLFilter::ProcessReadPlaintextBuffer(int start, int end) {
   int length = end - start;
   int bytes_processed = 0;
   if (length > 0) {
     bytes_processed = SSL_read(
         ssl_,
         reinterpret_cast<char*>((buffers_[kReadPlaintext] + start)),
         length);
     if (bytes_processed < 0) {
@@ skipping 58 matching lines @@
     } else {
       if (SSL_LOG_DATA) Log::Print(
           "WriteEncrypted  BIO_read wrote %d bytes\n", bytes_processed);
     }
   }
   return bytes_processed;
 }
 
 }  // namespace bin
 }  // namespace dart