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 in error handle.

[Bill Hesse, 2016/03/01 17:02:18]

"if handle is an error handle"?

[zra, 2016/03/01 21:19:02]

Done.

+#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)) {
+    delete filter;

[Bill Hesse, 2016/03/01 17:02:18]

Won't the delete filter from the WeakPersistentHandle created in SetFilter also
be called if we hit this case? 

[zra, 2016/03/01 21:19:02]

Removed

+    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);
   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);
+}
+
+
 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);

[Bill Hesse, 2016/03/01 17:02:18]

Can't we move this inside the call to WrappedX509Certificate, just before
calling NewWeakPersistentHandle?  Everything gets a lot simpler.  But we would
have to decrement the count in line 1365, in PeerCertificate.

[zra, 2016/03/01 21:19:02]

Scared to dec the refcount in PeerCertificate. It shouldn't do anything weird,
but who knows what the implementation will do if the count drops to one or zero
before we get a chance to inc it again. I'll leave this here, and add a comment
to WrappedX509Certificate.

+  }
   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);
+  Dart_Handle err = InitializeBuffers(dart_this);
+  if (Dart_IsError(err)) {
+    Dart_DeletePersistentHandle(string_start_);

[Bill Hesse, 2016/03/01 17:02:18]

If we put a check for null on buffers[i] in Destroy, we can just call Destroy
here, can't we?

[zra, 2016/03/01 21:19:02]

Done.

+    string_start_ = NULL;
+    Dart_DeletePersistentHandle(string_length_);
+    string_length_ = NULL;
+    Dart_DeletePersistentHandle(bad_certificate_callback_);
+    bad_certificate_callback_ = NULL;
+  }
+  return err;
 }
 
 
-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;
+    }
   }
+
+  // Clean up if there was an error.
+  if (Dart_IsError(result)) {
+    for (int i = 0; i < kNumBuffers; i++) {
+      if (dart_buffer_objects_[i] != NULL) {

[Bill Hesse, 2016/03/01 17:02:18]

Can we add this checked cleanup to Destroy, and just call Destroy if we get an
error handle returned back to Init?

[zra, 2016/03/01 21:19:02]

Done.

+        Dart_DeletePersistentHandle(dart_buffer_objects_[i]);
+      }
+      delete buffers_[i];
+      buffers_[i] = NULL;
+    }
+    return result;
+  }
+  return Dart_Null();
 }
 
 
 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];
+    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) {
+    Dart_DeletePersistentHandle(dart_buffer_objects_[i]);
+    dart_buffer_objects_[i] = NULL;
+  }
+  Dart_DeletePersistentHandle(string_start_);
+  string_start_ = NULL;
+  Dart_DeletePersistentHandle(string_length_);
+  string_length_ = NULL;
+  Dart_DeletePersistentHandle(handshake_complete_);
+  handshake_complete_ = 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