dart:io | Enable multithreaded secure networking encryption.

dart:io | Enable multithreaded secure networking encryption.

BUG=
R=ajohnsen@google.com, sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24233

[Anders Johnsen, 2013-06-13 13:14:17]

Can you provide some overall information about this change?

https://codereview.chromium.org/16858011/diff/1/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/16858011/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:299: List<int> bufferedData,
Can you factor out this change?

[Bill Hesse, 2013-06-13 14:59:29]

Here is the change to make encryption and decryption of secure socket data
happen on  separate threads, asynchronously.

Instead of the RawSecureSocket calling the native ProcessBuffer function, which
pushes data through the encryption filter synchronously before returning, it
calls _runFilter, which calls _pushAllFilterStages to use the native port
calling convention to run the C++ ProcessFilter function on a separate thread.

We also switch to circular buffers, to communicate between C++ and Dart.  Each
buffer is either read by Dart and written by C++, or vice versa.  This allows
concurrent use of the buffers, with reading and writing at the same time, and
synchronization happening when the C++ function returns a message through the
port to Dart.

Most events and changes of state happen when the C++ function returns new values
to Dart, which happens asynchronously.  Therefore, many of them have moved from
the event handlers and public functions to the second halves of _tryFilter and
_PushAllFilterStages.

There is also a simplification of the loop that continues to send READ events
while there is decrypted data available, using pendingRead to track whether
there is a READ event already scheduled to be sent.  This also fixes the tests
that drop READ events, since it guarantees they are sent continually, while read
events are enabled.

[Anders Johnsen, 2013-06-14 06:54:41]

I'm wondering if it would be possible to separate the socket-logic and the
buffer/filter logic. Currently, it can be a bit difficult to understand the
state, as it happens side-by-side.

LGTM, but wait for sgjesse's comments, as he knows more about this than me.

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

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:285: int size = isEncrypted(i) ?
encrypted_buffer_size_ : buffer_size_;
isEncrypted -> IsBufferEncrypted.

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket_...
File runtime/bin/secure_socket_patch.dart (right):

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket_...
runtime/bin/secure_socket_patch.dart:96: int _pointer() native
"SecureSocket_FilterPointer";
Add comment that this is a security issue, as mirrors can call this and get the
pointer.

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:359: static bool isEncrypted(int identifier) =>
identifier >= READ_ENCRYPTED;
isBufferEncrypted

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:636: _runFilter();
Don't you mean to _runFilter inside the loop? Otherwise, the loop could be
removed?

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:665: _runFilter();
Ditto.

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:755: } catch (e) {
what errors are you expecting to catch here?

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:857: List<int> readSocketOrBufferedData(int bytes)
{
Make private.

[Bill Hesse, 2013-06-14 08:55:02]

New version addressing comments uploaded.

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

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:285: int size = isEncrypted(i) ?
encrypted_buffer_size_ : buffer_size_;
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> isEncrypted -> IsBufferEncrypted.

Done.

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket_...
File runtime/bin/secure_socket_patch.dart (right):

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket_...
runtime/bin/secure_socket_patch.dart:96: int _pointer() native
"SecureSocket_FilterPointer";
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> Add comment that this is a security issue, as mirrors can call this and get
the
> pointer.

Done.

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:359: static bool isEncrypted(int identifier) =>
identifier >= READ_ENCRYPTED;
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> isBufferEncrypted

Changed to 
_isBufferEncrypted

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:636: _runFilter();
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> Don't you mean to _runFilter inside the loop? Otherwise, the loop could be
> removed?

No.  The loop runs at most twice, for the two existing linear ranges of data in
the circular buffer.

_runFilter() is asynchronous, and just starts a processFilter call if none is
already running.  Maybe we should rename it _scheduleFilter.

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:665: _runFilter();
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> Ditto.

Done.

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:755: } catch (e) {
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> what errors are you expecting to catch here?

All handshake errors are caught here - we used to have try-catch blocks
surrounding each call to _secureHandshake.  Also any errors from RawSocket.read
and RawSocket.write on the underlying socket.

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:857: List<int> readSocketOrBufferedData(int bytes)
{
On 2013/06/14 06:54:41, Anders Johnsen wrote:
> Make private.

Done.

[Søren Gjesse, 2013-06-14 09:27:44]

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

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:260: bool in_handshake =
CObjectBool(args[1]).Value();
How about adding a C++ class for the circular buffer

class CircularBuffer {
 public:
  CircularBuffer(char* data, int start, int end) ...
  
  bool isEmpty ...
  void ProcessedBytes(int bytes) ...
  void AddedBytes(int bytes) ...

 private:
  char* data;
  int start;
  int end;
}

and use that instead of the start/end arrays? I think it could be easier to
read.

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:282: if (in_handshake && (i == kReadPlaintext || i
== kWritePlaintext)) continue;
Is the is_handshake boolean needed? During handshake kReadPlaintext will be
empty, and kWritePlaintext should also be empty as the Dart RawSecureSocket
object is not available before handshake is done.

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:725: // The kWritePlaintext case must be called to
push data through the filter.
Why is it that calling PR_Write with a length of 0 has an effect?

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:624: if (length == 0) return null;
Move this code into the buffer in a method called get (or read) that returns the
new list.

  List<int> get(length)

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:655: int written = 0;
Move this code into the buffer in a method called add (or write) that returns
written.

  int add(data, offset, bytes)

https://codereview.chromium.org/16858011/diff/5001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:1024: class _ExternalBuffer {
I think adding more methods to this class would make it easier to understand the
code. E.g. make advanceStart and advanceEnd "private" and add methods to
actually add and get bytes.

If the C++ class used in the native code could have an identical interface that
would be great.

[Bill Hesse, 2013-06-14 12:28:19]

Made the suggested changes to the Dart code.  I'll now see if the C++ code can
be changed similarly.  The Dart code is ready for review.

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

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:282: if (in_handshake && (i == kReadPlaintext || i
== kWritePlaintext)) continue;
On 2013/06/14 09:27:44, Søren Gjesse wrote:
> Is the is_handshake boolean needed? During handshake kReadPlaintext will be
> empty, and kWritePlaintext should also be empty as the Dart RawSecureSocket
> object is not available before handshake is done.

kReadPlaintext is not full, so the filter would try to read to it, and that
breaks the filter if the handshake is not complete.

https://codereview.chromium.org/16858011/diff/5001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:725: // The kWritePlaintext case must be called to
push data through the filter.
On 2013/06/14 09:27:44, Søren Gjesse wrote:
> Why is it that calling PR_Write with a length of 0 has an effect?

The memio_PutWriteResult call in the kWriteEncrypted case will not pull data
through the encryption filter to be read from the memio object.  We need calls
to PR_Write, even if no new data is being added, until the write channel is
empty.

[Bill Hesse, 2013-06-14 13:47:09]

Added helper functions to _pushAllFilterStages.  After discussion, the C++ code
would not benefit from creating a class to abstract the circular buffer.

[Søren Gjesse, 2013-06-17 07:31:07]

LGTM!

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

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:233: /**
Nice comment

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:264: starts[i] = CObjectInt32(args[2*i +
2]).Value();
We normally have space around all binary operators.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:270: for (int i = 0; i < SSLFilter::kNumBuffers;
++i) {
You could "cheat" here and just update the int value of the existing
Dart_CObjects instead of allocating new ones. You are already re-using the
request array.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:288: case kWriteEncrypted:
Maybe add an overall comment here: "Write data to buffer free space".

Also document that if the buffer is already full nothing will happen. Maybe add
an initial for full buffer and just continue in that case.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:289: if (start <= end) {
If start == 0 this is not two segments after all.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:290: // Free space is split into two segments.
Please explain the "or" (when is the last element not incuded).

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:306: case kWritePlaintext:
Overall comment here: "Read data from buffer"

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:722: intptr_t SSLFilter::ProcessBuffer(int
buffer_index, int start, int end) {
You don't need for this CL, but how about just splitting this function into for
functions:

  ProcessWritePlaintext
  ProcessReadPlaintext
  ...

And calling each of these from ProcessAllBuffers?

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:738: // TODO(whesse): Handle errors
Please open an issue on this.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket_...
File runtime/bin/secure_socket_patch.dart (right):

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket_...
runtime/bin/secure_socket_patch.dart:56: // in the encrypted buffer, when
encrypted.
We should do some experiments with these buffer sizes. How do well do they fit
with the streaming through a HTTPS connection?

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:934: // because the handshake wrote data after we
checked.
Just checking the boolean position[1] looks a bit magical. How about just adding

  bool wasInHandshake() => positions[1] like start/end above.

Consider renaming positions to response as it is more than just the positions.

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:1044: while (bytesRead < bytes) {  // Loop over
two linear data ranges.
two -> one or two.

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:1062: while (toWrite > 0) {  // Loop over two
linear free ranges.
Ditto.

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:1103: int end;
size should be final.

[Bill Hesse, 2013-06-20 14:56:26]

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

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:233: /**
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> Nice comment

Thanks a lot

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:264: starts[i] = CObjectInt32(args[2*i +
2]).Value();
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> We normally have space around all binary operators.

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:270: for (int i = 0; i < SSLFilter::kNumBuffers;
++i) {
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> You could "cheat" here and just update the int value of the existing
> Dart_CObjects instead of allocating new ones. You are already re-using the
> request array.

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:288: case kWriteEncrypted:
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> Maybe add an overall comment here: "Write data to buffer free space".
> 
> Also document that if the buffer is already full nothing will happen. Maybe
add
> an initial for full buffer and just continue in that case.

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:289: if (start <= end) {
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> If start == 0 this is not two segments after all.
 Or if it equals 1.  I think the comments are enough now.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:290: // Free space is split into two segments.
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> Please explain the "or" (when is the last element not incuded).

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:306: case kWritePlaintext:
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> Overall comment here: "Read data from buffer"

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:722: intptr_t SSLFilter::ProcessBuffer(int
buffer_index, int start, int end) {
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> You don't need for this CL, but how about just splitting this function into
for
> functions:
> 
>   ProcessWritePlaintext
>   ProcessReadPlaintext
>   ...
> 
> And calling each of these from ProcessAllBuffers?

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:738: // TODO(whesse): Handle errors
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> Please open an issue on this.

Done.

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket_...
File runtime/bin/secure_socket_patch.dart (right):

https://codereview.chromium.org/16858011/diff/9002/runtime/bin/secure_socket_...
runtime/bin/secure_socket_patch.dart:56: // in the encrypted buffer, when
encrypted.
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> We should do some experiments with these buffer sizes. How do well do they fit
> with the streaming through a HTTPS connection? 

Yes, we should.

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:934: // because the handshake wrote data after we
checked.
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> Just checking the boolean position[1] looks a bit magical. How about just
adding
> 
>   bool wasInHandshake() => positions[1] like start/end above.
> 
> Consider renaming positions to response as it is more than just the positions.

Done.

I made the function wasInHandshake(), wished I could make it a getter, and then
realized it was just a final bool variable.

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:1044: while (bytesRead < bytes) {  // Loop over
two linear data ranges.
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> two -> one or two.

Done.

https://codereview.chromium.org/16858011/diff/9002/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:1103: int end;
On 2013/06/17 07:31:07, Søren Gjesse wrote:
> size should be final.

Done.

[Bill Hesse, 2013-06-20 15:06:54]

Committed patchset #6 manually as r24233 (presubmit successful).