Fixes error handling, leaks in secure sockets.
- Peer certificate was leaked, cert passed to bad cert callback could
could become stale.
- Added finalizer.
- Failing to call Destroy would leak various things.
- Added finalizer.
- ThrowIfError in initialization would fail to deallocate various
things on an error.
- Replaced with explicit checks, and deallocation where needed.
R=iposva@google.com, whesse@google.com
Review URL: https://codereview.chromium.org/1746363002 .
diff --git a/runtime/bin/secure_socket.cc b/runtime/bin/secure_socket.cc
index f63d9d0..7d79dc6 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 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 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 @@
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_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.
filter->Destroy();
- delete filter;
}
@@ -271,7 +314,8 @@
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 @@
}
+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 @@
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 @@
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 @@
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 @@
}
-void SSLFilter::Init(Dart_Handle dart_this) {
+Dart_Handle SSLFilter::Init(Dart_Handle dart_this) {
if (!library_initialized_) {
InitializeLibrary();
}
@@ -1168,24 +1241,40 @@
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 @@
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 @@
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::Destroy() {
+SSLFilter::~SSLFilter() {
if (ssl_ != NULL) {
SSL_free(ssl_);
ssl_ = NULL;
@@ -1498,13 +1608,37 @@
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;
+ }
}
diff --git a/runtime/bin/secure_socket.h b/runtime/bin/secure_socket.h
index bd5eda3..754d240 100644
--- a/runtime/bin/secure_socket.h
+++ b/runtime/bin/secure_socket.h
@@ -62,7 +62,9 @@
in_handshake_(false),
hostname_(NULL) { }
- void Init(Dart_Handle dart_this);
+ ~SSLFilter();
+
+ Dart_Handle Init(Dart_Handle dart_this);
void Connect(const char* hostname,
SSL_CTX* context,
bool is_server,
@@ -120,7 +122,7 @@
static bool isBufferEncrypted(int i) {
return static_cast<BufferIndex>(i) >= kFirstEncrypted;
}
- void InitializeBuffers(Dart_Handle dart_this);
+ Dart_Handle InitializeBuffers(Dart_Handle dart_this);
void InitializePlatformData();
DISALLOW_COPY_AND_ASSIGN(SSLFilter);
