When reading from an SSL socket, attempt to fully fill the caller's buffer

When reading from an SSL socket, attempt to fully fill the caller's buffer

The current SSLClientSocket implementation reads one SSL
record at a time, and immediately returns that to the caller
of Read(). As it is a common performance optimization to set
SSL record sizes to fit within MTU, this leads to suboptimal
performance and causes SSLClientSocket::Read() to not
match the behaviour of TCPClientSocket::Read() (which
attempts to fully fill the caller's buffer).

BUG=166903
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182912

[Ryan Sleevi, 2013-02-13 01:21:16]

rch, wtc: Because of the subtlety and my nervousness here, I'm asking for a
double review :)

wtc: Can you check the implementation, and think about potential
issues/side-effects that may exist (eg: renegotiation is a big concern of mine)

rch: Can you check the test, and see if you have any suggestions on negative
test cases or other tests that should be added.

Note: Renegotiation is not tested right now because tlslite does not support
renegotiation (I was talking to Trevor a few weeks ago about adding it, but it's
been a while since I've been deep in the TLSlite implementation. We also need to
upgrade to a version not ~7 years old).

Note: The net:: overkill in the test is bothersome, and in a follow-up CL, I
will move the tests to an unnamed namespace within net:: so that all the net::
bits can be dropped. I did not want to over-complicate this CL.

[Ryan Hamilton, 2013-02-13 17:23:17]

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2009: pending_read_result_ = rv;
same comment about using a const kNoPendingReadResult = 1.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:1365: pending_read_error_ = rv;
It looks like this is the only place you assign a "success" value to
pending_read_error_ that is not "1".  I think I would be inclined to create a
const int kNoPeindingError = 1 and use it for clarity.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:703: net::TestServer
test_server(net::TestServer::TYPE_HTTPS,
as you said, the proliferation of net:: is ... in need of deletion :>

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:744: transport->SetBufferSize(15000);
SO this line means that the transport wrapper socket will read 15K from the wire
before it allows the first read to return, right?  At that point, it will allow
Read() to return synchronously for 15K.  Do I have that right?  If so, why 15K,
not 17K?

Also, this test is called ManySmallReads, but I'm not quite sure that I
understand where "many" and "small" are coming from?  I suspect your test is
doing the right thing, but I'm missing it...

[Ryan Sleevi, 2013-02-13 20:44:43]

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:1365: pending_read_error_ = rv;
On 2013/02/13 17:23:18, Ryan Hamilton wrote:
> It looks like this is the only place you assign a "success" value to
> pending_read_error_ that is not "1".  I think I would be inclined to create a
> const int kNoPeindingError = 1 and use it for clarity.

Actually, this is a "failure" value (it can only be reached if we broke on line
1358 with rv being <= 0. I will add a comment to explain that.

That said, using the constant is both reasonable and preferable, so I will make
that change.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:744: transport->SetBufferSize(15000);
On 2013/02/13 17:23:18, Ryan Hamilton wrote:
> SO this line means that the transport wrapper socket will read 15K from the
wire
> before it allows the first read to return, right?  At that point, it will
allow
> Read() to return synchronously for 15K.  Do I have that right?  If so, why
15K,
> not 17K?

Yes, it means at least 15K must be read/buffered before returning. This is true
even if the Read() request is only for 1 byte (it won't be - it will either be
for 17K or 16K, depending on how quickly the handshake completed w/r/t false
start)

I will add a comment explaining this bit - that it's both contingent upon false
start and contingent upon our first read in Read() potentially attempting to
Read() a partial buffer (< 17K). 15K was chosen simply because the SSL overhead
will have the 8K PT we try to read always be < than the 15K we have buffered,
and the 8K will be > than the 1350 (plaintext) byte records we're supplying in
the Python script.

> 
> Also, this test is called ManySmallReads, but I'm not quite sure that I
> understand where "many" and "small" are coming from?  I suspect your test is
> doing the right thing, but I'm missing it...

The test is called ManySmallRecords - not ManySmallReads. The "Records" are
"many small SSL records" (1350 byte records - consistent with a certain large
SSL deployment), and attempting to read 8K.

[Ryan Hamilton, 2013-02-13 21:50:34]

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:1365: pending_read_error_ = rv;
On 2013/02/13 20:44:43, Ryan Sleevi wrote:
> On 2013/02/13 17:23:18, Ryan Hamilton wrote:
> > It looks like this is the only place you assign a "success" value to
> > pending_read_error_ that is not "1".  I think I would be inclined to create
a
> > const int kNoPeindingError = 1 and use it for clarity.
> 
> Actually, this is a "failure" value (it can only be reached if we broke on
line
> 1358 with rv being <= 0. I will add a comment to explain that.
> 
> That said, using the constant is both reasonable and preferable, so I will
make
> that change.

As part of this CL, or in a different one?

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:744: transport->SetBufferSize(15000);
On 2013/02/13 20:44:43, Ryan Sleevi wrote:
> On 2013/02/13 17:23:18, Ryan Hamilton wrote:
> > SO this line means that the transport wrapper socket will read 15K from the
> wire
> > before it allows the first read to return, right?  At that point, it will
> allow
> > Read() to return synchronously for 15K.  Do I have that right?  If so, why
> 15K,
> > not 17K?
> 
> Yes, it means at least 15K must be read/buffered before returning. This is
true
> even if the Read() request is only for 1 byte (it won't be - it will either be
> for 17K or 16K, depending on how quickly the handshake completed w/r/t false
> start)
> 
> I will add a comment explaining this bit - that it's both contingent upon
false
> start and contingent upon our first read in Read() potentially attempting to
> Read() a partial buffer (< 17K). 15K was chosen simply because the SSL
overhead
> will have the 8K PT we try to read always be < than the 15K we have buffered,
> and the 8K will be > than the 1350 (plaintext) byte records we're supplying in
> the Python script.

Sounds great.

> The test is called ManySmallRecords - not ManySmallReads. The "Records" are
> "many small SSL records" (1350 byte records - consistent with a certain large
> SSL deployment), and attempting to read 8K.

Are you saying I can't read?  Huh?!  Is that what you're saying?!  Because if
that's what you're saying...

it's pretty clearly true. :>  Sorry for the confusion!  Thanks for the
explanation.

[wtc, 2013-02-13 22:06:51]

Patch set 5 LGTM.

I read ssl_client_socket_nss.cc very carefully. Many of my
comments on ssl_client_socket_nss.cc also apply to
ssl_client_socket_openssl.cc. For brevity, I didn't repeat
the comments.

I also did a good review of ssl_client_socket_openssl.{h,cc}.

The two test-related files I just skimmed through.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (left):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1951: int amount_in_read_buffer =
memio_GetReadableBufferSize(nss_bufs_);

Just wanted to verify that you meant to delete this
memio_GetReadableBufferSize call.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:841: // buffer.

The goal should be: when data has been received, fill the
caller's buffer as much as possible *without blocking*.
"completely fill the caller's buffer" seems to imply that
we are willing to wait. I suggest adding "without blocking"
to the end of this comment. But perhaps "without blocking"
is obvious when people read the DoPayloadRead() code.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:847: PRErrorCode pending_read_prerror_;

prerror => nss_error

Document that this member is only meaningful when
pending_read_result_ is < 0.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1969: if (rv != ERR_IO_PENDING) {

We should add && rv != 0. A graceful connection closure
should not cause a NetLog::TYPE_SSL_READ_ERROR event to be
logged. The current code logs a
NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED event if rv == 0.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1980: // requested. Normally, PR_Read will
return exactly one SSL application data

Please add "in the current NSS implementation". We should
eventually change this NSS behavior.

It may be useful to mention the potential 1-byte
application data records if the server does 1/n-1 record
splitting.

Typo: noticably => noticeably

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1988: // should be returned immediately.

I believe that if renegotiation happens, PR_Read will return
-1 with PR_WOULD_BLOCK_ERROR. So it should be transparent to
us.

EOF (zero return from PR_Read) should not be referred to as
an error.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2007: int* next_result = &rv;

Just a commentary: the use of next_result makes this code a
little hard to understand.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2015: *next_result =
ERR_SSL_CLIENT_AUTH_CERT_NEEDED;

We should set pending_read_prerror_ to 0 in this case,
especially if next_result is  &pending_read_prerror_.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2022: pending_read_prerror_ = 0;

It seems OK for pending_read_prerror_ to be
PR_WOULD_BLOCK_ERROR in this case.

IMPORTANT: I think it is better to set *next_result to 1 if next_result
points to &pending_read_result_. Otherwise we will waste
the next DoPayloadRead() call doing that (see lines 1963-1977).

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:1348: pending_read_error_ = 1;
If rv == 0, we need to log a NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED
event to match the original code.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:54: void SetBufferSize(int size);

Should |size| have the size_t type?

[Ryan Sleevi, 2013-02-13 22:55:48]

Thanks for your careful reviews. I've uploaded Patchset 6 to address these
comments.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (left):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1951: int amount_in_read_buffer =
memio_GetReadableBufferSize(nss_bufs_);
On 2013/02/13 22:06:51, wtc wrote:
> 
> Just wanted to verify that you meant to delete this
> memio_GetReadableBufferSize call.

Nope. Rebase bug. Good catch.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1980: // requested. Normally, PR_Read will
return exactly one SSL application data
On 2013/02/13 22:06:51, wtc wrote:
> 
> Please add "in the current NSS implementation". We should
> eventually change this NSS behavior.

I'm not sure I agree, for reasons I explained previously, particularly as it
relates to renegotiation.

For example, if we greedily read to the point of re-entering a handshake state,
we lose the ability to process or access any handshake state (such as the
certificates) associated with the data received. Having the SSL library return
'record at a time' grants the application the control to access or respond
according to data received (whether server or client).

> 
> It may be useful to mention the potential 1-byte
> application data records if the server does 1/n-1 record
> splitting.
> 
> Typo: noticably => noticeably

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1988: // should be returned immediately.
On 2013/02/13 22:06:51, wtc wrote:
> 
> I believe that if renegotiation happens, PR_Read will return
> -1 with PR_WOULD_BLOCK_ERROR. So it should be transparent to
> us.
> 

My focus was in highlighting that SSL is not always "call and response" (eg:
WebSockets), so things like renegotiation cause writes to happen in read and
would prevent us from reading 'all' the data in the buffer.

> EOF (zero return from PR_Read) should not be referred to as
> an error.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2022: pending_read_prerror_ = 0;
On 2013/02/13 22:06:51, wtc wrote:
> 
> It seems OK for pending_read_prerror_ to be
> PR_WOULD_BLOCK_ERROR in this case.
> 
> IMPORTANT: I think it is better to set *next_result to 1 if next_result
> points to &pending_read_result_. Otherwise we will waste
> the next DoPayloadRead() call doing that (see lines 1963-1977).

I'm not sure I fully follow, but what I understand you to be suggesting, I think
would be buggy.

The goal here is to make sure that control is returned to the caller to indicate
ERR_IO_PENDING - that is, there is an operation pending. This is handled on line
1976.

If we set pending_read_result_ = kNoPendingReadResult, then we would not return
early - thus calling PR_Read again, before control of the underlying transport
had returned any (new) data for PR_Read to use. Thus we'd still end up in
pending, but we'd have spent even more time in the function.

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:54: void SetBufferSize(int size);
On 2013/02/13 22:06:51, wtc wrote:
> 
> Should |size| have the size_t type?

In theory, yes, but consistent with the entire Socket interface (and net/ in
general), this is kept as an int for size.

[Ryan Hamilton, 2013-02-13 23:17:43]

lgtm

https://codereview.chromium.org/12025040/diff/13006/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/12025040/diff/13006/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:746: // of ciphertext necessary to
contain the 8K of plaintext requested below.
nice comment.  very clear now.

[Ryan Sleevi, 2013-02-15 22:27:47]

Wan-Teh swung by and explained the error in Patchset 7, which was originally
flagged as performance, but actually would be a bug when running on
multi-threaded. This explains why the Linux bots were failing. I raised a
concern to the troopers that the test results appear to be getting buffered, as
it should have shown that the hang was due to the SSLClientSocket tests - not
the URLRequest tests (which are over HTTP), as it was reporting.

Wan-Teh, did you want to look over the diff between PS7 & 8 and make sure you're
happy with the comment?

[wtc, 2013-02-15 23:14:49]

Patch set 9 LGTM. I only reviewed ssl_client_socket_nss.cc
and ssl_client_socket_openssl.cc.

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:152: // obtained by a previous operation
waiting to be returned to the caller.

It'd be nice to point out this constant can be any nonnegative
value so it can be distinguished from EOF and errors.

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1981: } else if (rv != ERR_IO_PENDING) {

I believe rv cannot be ERR_IO_PENDING.

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2055: *next_result = ERR_IO_PENDING;

I think it's clearer to use |rv| here:
    rv = ERR_IO_PENDING;

Note that you are already using |rv| and |pending_read_result_|
directly
in this code block.

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2058: *next_result =
HandleNSSError(pending_read_nss_error_, false);

HandleNSSError maps PR_WOULD_BLOCK_ERROR to ERR_IO_PENDING,
so you should be able to use the simpler control structure
that you used in the equivalent code in
ssl_client_socket_openssl.cc.

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2062: 

Here you can CHECK or DCHECK that we never set
pending_read_result_ to ERR_IO_PENDING.

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:1370: // thread-local storage. However,
the handled/remapped error code should

SSL_get_error() takes ssl_ as input. So perhaps the OpenSSL
error is stored in ssl_ as opposed to thread-local storage?

[wtc, 2013-02-15 23:30:04]

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12025040/diff/19001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:1980: // requested. Normally, PR_Read will
return exactly one SSL application data

On 2013/02/13 22:55:48, Ryan Sleevi wrote:
>
> I'm not sure I agree, for reasons I explained previously, particularly as it
> relates to renegotiation.

The techniques you're using here (calling PR_Read in a loop
and saving a pending read result in a data member) can also
be done inside NSS. This way all NSS users will benefit.
(This also applies to OpenSSL.)

NSS will need to take care to not do greedy read for
either DTLS or a blocking-mode SSL socket, but NSS has
access to internal state, which may enable it to do a better
job.

Re: renegotiation: SSL/TLS allows application data records
to be sent during a renegotiation, so an application needs
to use the NSS "handshake hook" to delineate data encrypted
using the old keys and data encrypted using the new keys.
In practice I don't think anyone is sending data during a
renegotiation, so an application can also use the NSS
"certificate auth hook" to delineate data encrypted using
different keys.

If NSS does the greedy read itself, it can also take care
to not return data encrypted with different keys in the
same PR_Read call. NSS knows when a renegotiation is done,
so it may be able to delineate the data more easily.

[Ryan Sleevi, 2013-02-16 00:20:16]

Thanks for the careful review, Wan-Teh!

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12025040/diff/47001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:1370: // thread-local storage. However,
the handled/remapped error code should
On 2013/02/15 23:14:49, wtc wrote:
> 
> SSL_get_error() takes ssl_ as input. So perhaps the OpenSSL
> error is stored in ssl_ as opposed to thread-local storage?

No, SSL_get_error() uses the ERR stack (eg: ERR_peek_error()) to see if there is
an error on the thread-local 'global' error stack (eg: FIPS errors, syscalls,
etc) before checking for "protocol" errors.

[commit-bot: I haz the power, 2013-02-16 00:20:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/12025040/45002

[commit-bot: I haz the power, 2013-02-16 04:10:12]

Change committed as 182912