Skip to content

Commit

Permalink
Improve TLS 1.3 support by always reading complete receive buffer
Browse files Browse the repository at this point in the history
Construct underlying stream to always consume complete receive buffer.
This avoids stale data in TLS buffers and also works around possible
buffering issues in legacy PHP versions. The buffer size is limited
due to TCP/IP buffers anyway, so this should not affect usage otherwise.

This builds on top of reactphp/stream#139 to
work around a bug in PHP where reading from a TLS 1.3 stream resource
would hang with 100% CPU usage due to the changed TLS 1.3 handshake.
  • Loading branch information
clue committed Jan 2, 2019
1 parent bfef257 commit e02071c
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 24 deletions.
13 changes: 0 additions & 13 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1372,19 +1372,6 @@ This library does not take responsibility over these context options, so it's
up to consumers of this library to take care of setting appropriate context
options as described above.

All versions of PHP prior to 5.6.8 suffered from a buffering issue where reading
from a streaming TLS connection could be one `data` event behind.
This library implements a work-around to try to flush the complete incoming
data buffers on these legacy PHP versions, which has a penalty of around 10% of
throughput on all connections.
With this work-around, we have not been able to reproduce this issue anymore,
but we have seen reports of people saying this could still affect some of the
older PHP versions (`5.5.23`, `5.6.7`, and `5.6.8`).
Note that this only affects *some* higher-level streaming protocols, such as
IRC over TLS, but should not affect HTTP over TLS (HTTPS).
Further investigation of this issue is needed.
For more insights, this issue is also covered by our test suite.

PHP < 7.1.4 (and PHP < 7.0.18) suffers from a bug when writing big
chunks of data over TLS streams at once.
We try to work around this by limiting the write chunk size to 8192
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"evenement/evenement": "^3.0 || ^2.0 || ^1.0",
"react/dns": "^0.4.13",
"react/event-loop": "^1.0 || ^0.5 || ^0.4 || ^0.3.5",
"react/stream": "^1.0 || ^0.7.1",
"react/stream": "^1.1",
"react/promise": "^2.6.0 || ^1.2.1",
"react/promise-timer": "^1.4.0"
},
Expand Down
15 changes: 5 additions & 10 deletions src/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,6 @@ class Connection extends EventEmitter implements ConnectionInterface

public function __construct($resource, LoopInterface $loop)
{
// PHP < 5.6.8 suffers from a buffer indicator bug on secure TLS connections
// as a work-around we always read the complete buffer until its end.
// The buffer size is limited due to TCP/IP buffers anyway, so this
// should not affect usage otherwise.
// See https://bugs.php.net/bug.php?id=65137
// https://bugs.php.net/bug.php?id=41631
// https://github.com/reactphp/socket-client/issues/24
$clearCompleteBuffer = \PHP_VERSION_ID < 50608;

// PHP < 7.1.4 (and PHP < 7.0.18) suffers from a bug when writing big
// chunks of data over TLS streams at once.
// We try to work around this by limiting the write chunk size to 8192
Expand All @@ -62,10 +53,14 @@ public function __construct($resource, LoopInterface $loop)
// See https://github.com/reactphp/socket/issues/105
$limitWriteChunks = (\PHP_VERSION_ID < 70018 || (\PHP_VERSION_ID >= 70100 && \PHP_VERSION_ID < 70104));

// Construct underlying stream to always consume complete receive buffer.
// This avoids stale data in TLS buffers and also works around possible
// buffering issues in legacy PHP versions. The buffer size is limited
// due to TCP/IP buffers anyway, so this should not affect usage otherwise.
$this->input = new DuplexResourceStream(
$resource,
$loop,
$clearCompleteBuffer ? -1 : null,
-1,
new WritableResourceStream($resource, $loop, null, $limitWriteChunks ? 8192 : null)
);

Expand Down

0 comments on commit e02071c

Please sign in to comment.