Fixes error handling, leaks in secure sockets.

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

Committed: https://github.com/dart-lang/sdk/commit/a28bfa98796cfc31676211f474b1c6e0ee68f776

[zra, 2016-02-29 23:51:38]

zra@google.com changed reviewers:
+ iposva@google.com, whesse@google.com

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

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:32: // Return the error from the containing
function if handle is in error handle.
"if handle is an error handle"?

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:211: delete filter;
Won't the delete filter from the WeakPersistentHandle created in SetFilter also
be called if we hit this case?

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:385: X509_up_ref(certificate);
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.

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:1235: Dart_DeletePersistentHandle(string_start_);
If we put a check for null on buffers[i] in Destroy, we can just call Destroy
here, can't we?

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:1323: if (dart_buffer_objects_[i] != NULL) {
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]

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:32: // Return the error from the containing
function if handle is in error handle.
On 2016/03/01 17:02:18, Bill Hesse wrote:
> "if handle is an error handle"?

Done.

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:211: delete filter;
On 2016/03/01 17:02:18, Bill Hesse wrote:
> Won't the delete filter from the WeakPersistentHandle created in SetFilter
also
> be called if we hit this case? 

Removed

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:385: X509_up_ref(certificate);
On 2016/03/01 17:02:18, Bill Hesse wrote:
> 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.

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.

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:1235: Dart_DeletePersistentHandle(string_start_);
On 2016/03/01 17:02:18, Bill Hesse wrote:
> If we put a check for null on buffers[i] in Destroy, we can just call Destroy
> here, can't we?

Done.

https://codereview.chromium.org/1746363002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:1323: if (dart_buffer_objects_[i] != NULL) {
On 2016/03/01 17:02:18, Bill Hesse wrote:
> Can we add this checked cleanup to Destroy, and just call Destroy if we get an
> error handle returned back to Init?

Done.

[zra, 2016-03-01 23:53:46]

@iposva

Did you have any comments?

[Bill Hesse, 2016-03-02 02:34:54]

lgtm

[Ivan Posva, 2016-03-02 04:52:52]

LGTM with 1 comment/question.

-ivan

https://codereview.chromium.org/1746363002/diff/40001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1746363002/diff/40001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:256: SSLFilter* filter = GetFilter(args);
When do we reset the native argument for the args being passed in? Do we end up
with a dangling "pointer" here?

If not, please explain in a comment. Thanks!

[zra, 2016-03-02 06:12:07]

https://codereview.chromium.org/1746363002/diff/40001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1746363002/diff/40001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:256: SSLFilter* filter = GetFilter(args);
On 2016/03/02 04:52:52, Ivan Posva wrote:
> When do we reset the native argument for the args being passed in? Do we end
up
> with a dangling "pointer" here?
> 
> If not, please explain in a comment. Thanks!

Done.

[zra, 2016-03-02 06:12:23]

Description was changed from

==========
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.
==========

to

==========
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

Committed:
https://github.com/dart-lang/sdk/commit/a28bfa98796cfc31676211f474b1c6e0ee68f776
==========

[zra, 2016-03-02 06:12:24]

Committed patchset #4 (id:60001) manually as
a28bfa98796cfc31676211f474b1c6e0ee68f776 (presubmit successful).

[Ivan Posva, 2016-03-02 06:17:22]

https://codereview.chromium.org/1746363002/diff/60001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1746363002/diff/60001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:262: // called more than once.
Thanks!