Start adding TLS (SSL) sockets to dart:io.

Start adding TLS (SSL) sockets to dart:io.

Add Linux implementation for SSL client sockets.



BUG=dart:3950
TEST=standalone/io/tls_socket_test

Committed: https://code.google.com/p/dart/source/detail?r=10565
Reverted: r=10567

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

[Bill Hesse, 2012-07-27 13:45:34]

I'm considering merging the two Dart classes.  Please take a look and tell me
what you think.  This is a big change, but I think this is close to an initial
commit.

[Anders Johnsen, 2012-07-27 15:58:49]

First round of comments. I have not looked at the logic yet, will comment on
that once these have been fixed.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:334: Dart_Handle
DartUtils::NewDartIllegalArgumentError(const char* message) {
Since this is pretty much a copy of the other message, can we create a unified
version that takes lib and exception name, that these two methods then uses?

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/dartutils.h
File runtime/bin/dartutils.h (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/dartutils.h#ne...
runtime/bin/dartutils.h:105: static Dart_Handle
NewDartIllegalArgumentError(const char* message);
Add comment.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/io.dart
File runtime/bin/io.dart (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/io.dart#newcode16
runtime/bin/io.dart:16: #import("dart:builtin");
Should not be here.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.dart
File runtime/bin/tls_socket.dart (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:47: * The write handler gets called when the socket
becomes available for
The logic should be the same as for any other Socket, so why is the comment
different?

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:96: void close([bool halfClose = false]);
Should not be here.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:8: #if defined(_WIN32)
Is this how we are supposed to do stuff like this? I would suggest either:
1) Have a util header for stuff like this.
2) Use stdint.h (if Google standards approve this, not sure).

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:6: int  _logging = true;
This, and below, does not belong here.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:20: static intptr_t Int64ToIntptr(int64_t x) {
Maybe move this to a util header, it look like something we would do often.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:38: printf("Entering TlsFilter constructor\n");
This file is full of printf statements.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:45: in_handshake = false;
Move to initializer list.

http://codereview.chromium.org/10704159/diff/16001/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10704159/diff/16001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:14: var tls = new
TlsSocket("www.google.dk", 443);
I'm unsure to if this is good practice or not - maybe add a TODO here, that we
should create and use a local host when we have a TlsServerSocket.

http://codereview.chromium.org/10704159/diff/16001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:32: print(received);
Print and out-commented code.

http://codereview.chromium.org/10704159/diff/16001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:39: // tls.close();
Ditto here.

[Søren Gjesse, 2012-07-31 09:07:15]

Some initial comments.

The overall structure looks fine.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:335: // Create a Dart OSError object with the
information retrieved from the OS.
This comment seems wrong.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/io.dart
File runtime/bin/io.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/io.dart#newcode16
runtime/bin/io.dart:16: #import("dart:builtin");
Please remove this before committing.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:29: 
Two empty lines functions.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.dart
File runtime/bin/tls_socket.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:12: 
As TlsSocket extends Socket is there any need to duplicate all the methods here
as well?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:10: typedef unsigned __int8 uint8_t;
We now have these typedefs in a number of places. Can't they be consolidated in
some central .h file, maybe platform/globals.h?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:25: static Dart_Handle HandleError(Dart_Handle handle)
{
Isn't this more of a function for dartutils.h?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:61: TlsFilterPlatformData* data;
Shouldn't all private variables end with and underscore and follow then
xxx_yyy_xxx_ naming convention?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:168: }
The 5 lines above here seems to be duplicated in several places. Could they be
refactored into a method on _TlsExternalBuffer? Perhaps combined with the
setRange into a "read"/"readList"/"readFrom" method.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:219: var free = _TlsExternalBuffer.kSize -
buffer.start - buffer.length;
Add this free calculation as a method on _TlsExternalBuffer?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:225: buffer.length += bytes;
Add an "add" method to _TlsExternalBuffer?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:27: 
Two empty lines between class declarations and between methods.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:121: bio_bytes = BIO_read(bio_stream,
Indentation of arguments.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:131: bio_bytes = BIO_write(bio_stream,
Ditto.

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:18: "GET / HTTP/1.0\r\nHost:
www.google.dk\r\n\r\n".charCodes();
Perhaps use HTTP/1.1 for the test.

[Mads Ager (google), 2012-07-31 10:16:01]

Drive-by comments. Overall, this is looking good.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/bin.gypi#newcode1
runtime/bin/bin.gypi:1: 
Please undo this change.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:23: Dart_Handle dart_this =
HandleError(Dart_GetNativeArgument(args, 0));
I would prefer to just handle the error explicitly here as we have done in all
the other files. I don't think the HandleError method adds anything here and you
should return here. But I think you are working on this already?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:122: printf("Exiting Init\n");
Please remove.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:10: typedef unsigned __int8 uint8_t;
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> We now have these typedefs in a number of places. Can't they be consolidated
in
> some central .h file, maybe platform/globals.h?

They are already there in platform/globals.h. They are all defined in
inttypes_support_win.h. Since platform/globals.h is included below we should be
able to remove all of this. Please also remove anywhere else where we have this
(I think file.h is the only other place?)

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:25: static Dart_Handle HandleError(Dart_Handle handle)
{
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Isn't this more of a function for dartutils.h?

I don't think we should have this at all?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:49: private:
Please use the '_' suffix for all private fields. Only peer_ has it.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:50: static const char* bufferNames[kNumBuffers];
No camel case in C++: buffer_names_[kNumBuffers];

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:6: int  _logging = true;
int?

We should remove this logging before comitting.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:8: void log(x) {
_log? We do not want to expose log to users.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:146: //    _readEncryptedData();
Code in comments.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:205: print('               
_fireCloseEventPending: $_fireCloseEventPending');
Remove.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:318: int length;
Do you need an extra length field here? Is this different from data.length?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:353: 
Remove this blank line?

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:38: printf("Entering TlsFilter constructor\n");
Remove all debug printing.

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:33: //    if
(isCompleteChunkedTransfer(message)) {
Code in comment.

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:39: // tls.close();
Code in comment.

[Bill Hesse, 2012-08-08 17:00:05]

OK.  All comments have been addressed, and there has been plenty of other clean
up.  The test should be committable, with windows and macos platforms skipped.

The InputStream and OutputStream interface to the socket is not yet implemented,
and certificate checking will be implemented later.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:334: Dart_Handle
DartUtils::NewDartIllegalArgumentError(const char* message) {
On 2012/07/27 15:58:49, ajohnsen wrote:
> Since this is pretty much a copy of the other message, can we create a unified
> version that takes lib and exception name, that these two methods then uses?

Done.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/io.dart
File runtime/bin/io.dart (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/io.dart#newcode16
runtime/bin/io.dart:16: #import("dart:builtin");
On 2012/07/27 15:58:49, ajohnsen wrote:
> Should not be here.

Done.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.dart
File runtime/bin/tls_socket.dart (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:47: * The write handler gets called when the socket
becomes available for
All of this code is removed.

On 2012/07/27 15:58:49, ajohnsen wrote:
> The logic should be the same as for any other Socket, so why is the comment
> different?

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:96: void close([bool halfClose = false]);
On 2012/07/27 15:58:49, ajohnsen wrote:
> Should not be here.

Done.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:8: #if defined(_WIN32)
No.  Removed.

On 2012/07/27 15:58:49, ajohnsen wrote:
> Is this how we are supposed to do stuff like this? I would suggest either:
> 1) Have a util header for stuff like this.
> 2) Use stdint.h (if Google standards approve this, not sure).

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:6: int  _logging = true;
On 2012/07/27 15:58:49, ajohnsen wrote:
> This, and below, does not belong here.

Done.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:38: printf("Entering TlsFilter constructor\n");
Removed.

On 2012/07/27 15:58:49, ajohnsen wrote:
> This file is full of printf statements.

http://codereview.chromium.org/10704159/diff/16001/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:45: in_handshake = false;
On 2012/07/27 15:58:49, ajohnsen wrote:
> Move to initializer list.

Done.

http://codereview.chromium.org/10704159/diff/16001/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10704159/diff/16001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:14: var tls = new
TlsSocket("www.google.dk", 443);
On 2012/07/27 15:58:49, ajohnsen wrote:
> I'm unsure to if this is good practice or not - maybe add a TODO here, that we
> should create and use a local host when we have a TlsServerSocket.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/bin.gypi#newcode1
runtime/bin/bin.gypi:1: 
On 2012/07/31 10:16:01, Mads Ager wrote:
> Please undo this change.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:335: // Create a Dart OSError object with the
information retrieved from the OS.
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> This comment seems wrong.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/io.dart
File runtime/bin/io.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/io.dart#newcode16
runtime/bin/io.dart:16: #import("dart:builtin");
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Please remove this before committing.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:23: Dart_Handle dart_this =
HandleError(Dart_GetNativeArgument(args, 0));
On 2012/07/31 10:16:01, Mads Ager wrote:
> I would prefer to just handle the error explicitly here as we have done in all
> the other files. I don't think the HandleError method adds anything here and
you
> should return here. But I think you are working on this already?

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:29: 
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Two empty lines functions.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:122: printf("Exiting Init\n");
On 2012/07/31 10:16:01, Mads Ager wrote:
> Please remove.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.dart
File runtime/bin/tls_socket.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:12: 
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> As TlsSocket extends Socket is there any need to duplicate all the methods
here
> as well?

No there is not.  Fixed.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:10: typedef unsigned __int8 uint8_t;
On 2012/07/31 10:16:01, Mads Ager wrote:
> On 2012/07/31 09:07:15, Søren Gjesse wrote:
> > We now have these typedefs in a number of places. Can't they be consolidated
> in
> > some central .h file, maybe platform/globals.h?
> 
> They are already there in platform/globals.h. They are all defined in
> inttypes_support_win.h. Since platform/globals.h is included below we should
be
> able to remove all of this. Please also remove anywhere else where we have
this
> (I think file.h is the only other place?)

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:49: private:
On 2012/07/31 10:16:01, Mads Ager wrote:
> Please use the '_' suffix for all private fields. Only peer_ has it.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:50: static const char* bufferNames[kNumBuffers];
On 2012/07/31 10:16:01, Mads Ager wrote:
> No camel case in C++: buffer_names_[kNumBuffers];

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:61: TlsFilterPlatformData* data;
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Shouldn't all private variables end with and underscore and follow then
> xxx_yyy_xxx_ naming convention?

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:6: int  _logging = true;
On 2012/07/31 10:16:01, Mads Ager wrote:
> int?
> 
> We should remove this logging before comitting.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:8: void log(x) {
On 2012/07/31 10:16:01, Mads Ager wrote:
> _log? We do not want to expose log to users.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:146: //    _readEncryptedData();
On 2012/07/31 10:16:01, Mads Ager wrote:
> Code in comments.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:168: }
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> The 5 lines above here seems to be duplicated in several places. Could they be
> refactored into a method on _TlsExternalBuffer? Perhaps combined with the
> setRange into a "read"/"readList"/"readFrom" method.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:205: print('               
_fireCloseEventPending: $_fireCloseEventPending');
On 2012/07/31 10:16:01, Mads Ager wrote:
> Remove.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:219: var free = _TlsExternalBuffer.kSize -
buffer.start - buffer.length;
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Add this free calculation as a method on _TlsExternalBuffer?

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:225: buffer.length += bytes;
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Add an "add" method to _TlsExternalBuffer?
 Because sometimes we set the range, and sometimes an external native function
sets the range, I think adding the advanceStart(int numBytes) method is
sufficient.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:318: int length;
On 2012/07/31 10:16:01, Mads Ager wrote:
> Do you need an extra length field here? Is this different from data.length?

Yes it is.  This is the length of the valid data region of the buffer.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:353: 
On 2012/07/31 10:16:01, Mads Ager wrote:
> Remove this blank line?
 These functions have been removed.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:27: 
On 2012/07/31 09:07:15, Søren Gjesse wrote:
> Two empty lines between class declarations and between methods.

Done.

http://codereview.chromium.org/10704159/diff/18015/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:38: printf("Entering TlsFilter constructor\n");
On 2012/07/31 10:16:01, Mads Ager wrote:
> Remove all debug printing.

Done.

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:33: //    if
(isCompleteChunkedTransfer(message)) {
On 2012/07/31 10:16:01, Mads Ager wrote:
> Code in comment.

Done.

http://codereview.chromium.org/10704159/diff/18015/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:39: // tls.close();
On 2012/07/31 10:16:01, Mads Ager wrote:
> Code in comment.

Done.

[Søren Gjesse, 2012-08-09 10:22:52]

LGTM with comments addressed

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/builtin_impl_s...
File runtime/bin/builtin_impl_sources.gypi (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/builtin_impl_s...
runtime/bin/builtin_impl_sources.gypi:67: 'tls_socket_linux.cc',
Don't we need empty implementations for Mac OS and Windows?

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:36: Dart_Handle dart_this =
HandleError(Dart_GetNativeArgument(args, 0));
Maybe add a function fir getting the local_data native field as it is done in
several places.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:122:
Dart_NewPersistentHandle(Dart_NewString("start")));
I don't see these persistent handles being deleted. Also if you need these two
strings can't they be more global than in this class?

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:149:
Dart_NewPersistentHandle(Dart_ListGetAt(dart_buffers_object, i)));
I don't see these persistent handles being deleted.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:78: _tlsFilter.connect();
Indent.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:78: intptr_t TlsFilter::ProcessBuffer(int
buffer_index) {
Wondering whether it would be easier to pass start and length to ProcessBuffer
instead of having to retrieve then from the corresponding Dart object.

[Bill Hesse, 2012-08-13 11:20:03]

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/builtin_impl_s...
File runtime/bin/builtin_impl_sources.gypi (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/builtin_impl_s...
runtime/bin/builtin_impl_sources.gypi:67: 'tls_socket_linux.cc',
On 2012/08/09 10:22:52, Søren Gjesse wrote:
> Don't we need empty implementations for Mac OS and Windows?

Done.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:36: Dart_Handle dart_this =
HandleError(Dart_GetNativeArgument(args, 0));
On 2012/08/09 10:22:52, Søren Gjesse wrote:
> Maybe add a function fir getting the local_data native field as it is done in
> several places.

Done.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:122:
Dart_NewPersistentHandle(Dart_NewString("start")));
Fixed - they are now freed in destroy().

On 2012/08/09 10:22:52, Søren Gjesse wrote:
> I don't see these persistent handles being deleted. Also if you need these two
> strings can't they be more global than in this class?

That's right, they aren't.  I'll fix that.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:149:
Dart_NewPersistentHandle(Dart_ListGetAt(dart_buffers_object, i)));
On 2012/08/09 10:22:52, Søren Gjesse wrote:
> I don't see these persistent handles being deleted.

Right. Fixed.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:78: _tlsFilter.connect();
On 2012/08/09 10:22:52, Søren Gjesse wrote:
> Indent.

Done.

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_lin...
File runtime/bin/tls_socket_linux.cc (right):

http://codereview.chromium.org/10704159/diff/13020/runtime/bin/tls_socket_lin...
runtime/bin/tls_socket_linux.cc:78: intptr_t TlsFilter::ProcessBuffer(int
buffer_index) {
I thought it was better to get them this way.  It should be pretty efficient.  I
guess we could benchmark.

On 2012/08/09 10:22:52, Søren Gjesse wrote:
> Wondering whether it would be easier to pass start and length to ProcessBuffer
> instead of having to retrieve then from the corresponding Dart object.

[siva, 2012-08-14 20:26:34]

This change has pretty much broken the linux build for all of us who were using
the gcc compiler from google3
/google/src/head/depot/google3/third_party/crosstool/v15/release
as there is no support for openssl in this cross tool chain.

The last time we had a dependency on openssl (bigint implementation) we had
included openssl in our source base under third_party.

[cshapiro, 2012-08-14 21:17:25]

DBC

https://chromiumcodereview.appspot.com/10704159/diff/24010/runtime/bin/tls_so...
File runtime/bin/tls_socket_android.cc (right):

https://chromiumcodereview.appspot.com/10704159/diff/24010/runtime/bin/tls_so...
runtime/bin/tls_socket_android.cc:10: class TlsFilterPlatformData {
There is no SSL library exposed by the Android NDK.  I believe Clank contains
its own copy of OpenSSL.

https://chromiumcodereview.appspot.com/10704159/diff/24010/runtime/bin/tls_so...
File runtime/bin/tls_socket_linux.cc (right):

https://chromiumcodereview.appspot.com/10704159/diff/24010/runtime/bin/tls_so...
runtime/bin/tls_socket_linux.cc:14: #include <openssl/ssl.h>
This adds a build dependency which we never had before.  If we have to depend on
a third party library, it would seem preferable to use NSS which I believe is
what Chromium depends on and documents in its machine preparation instructions
for Linux.

https://chromiumcodereview.appspot.com/10704159/diff/24010/runtime/bin/tls_so...
runtime/bin/tls_socket_linux.cc:125: }
No default case?  Is this even reachable?

If not, maybe

  if (buffer_index == kReadPlaintex || buffer_index == kWriteEncrypted) {
    ...
  } else {
    ASSERT(buffer_index == kReadEncryped || buffer_index == kWritePlaintex);
    ...
  }

There is a similar condition on line 96.

https://chromiumcodereview.appspot.com/10704159/diff/24010/runtime/bin/tls_so...
runtime/bin/tls_socket_linux.cc:127: bytes_processed = bio_bytes;
Is the indentation correct? The "b" in bytes_processed lines up underneath the
"b" in bio_bytes.

[Ivan Posva, 2012-08-15 04:36:43]

Bill,

Please revert this change as it causes build breaks on the platforms where TLS
sockets are currently implemented and there is no clear way forward yet on
Windows. We need a better plan for this before checking this in again.

Thanks,
-Ivan