Skip to content

Commit efaeb8d

Browse files
committed
Fix timeout after supplemental read of non-blocking stream
This issue is related to GH-22332: > Let's assume we have a stream with 5 queued bytes. When calling fread($s, 1), > the underlying buffered stream will request a chunk of up to 8192 bytes > immediately, but only return the requested 1 byte back to the reader. If the > reader then requests fread($s, 10), _php_stream_read() will first return > anything that was buffered but not yet read. > > If the requested length exceeds the number of buffered bytes (as is the case > above), another read call is issued. This call will return nothing, because > the stream only provides 4 more readable bytes, all of which are buffered. > php_openssl_handle_ssl_error() (called by php_openssl_sockop_io()) will then > incorrectly set last_status to WANT_READ, even though we've already read the > remaining data. > > Furthermore, stream_select() can cause the same issue via > php_openssl_sockop_cast(castas: PHP_STREAM_AS_FD_FOR_SELECT), which pre-fills > the read buffer on SSL_pending() > 0. The subsequent fread() will lead to the > same condition as above. > > There's a second issue here. If the stream is blocking, the supplement read > will block for the duration of the timeout. This will be addressed in a second > PR. This addresses the last paragraph. Avoid a blocking read when we already have buffered data, as we may have reached the end of the stream and will wait in vain.
1 parent a480965 commit efaeb8d

2 files changed

Lines changed: 98 additions & 4 deletions

File tree

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
--TEST--
2+
Supplemental read of blocking stream does not cause timeout
3+
--EXTENSIONS--
4+
openssl
5+
--SKIPIF--
6+
<?php
7+
if (!function_exists("proc_open")) die("skip no proc_open");
8+
?>
9+
--FILE--
10+
<?php
11+
$certFile = __DIR__ . DIRECTORY_SEPARATOR . 'crypto_supplemental_read_timeout.pem.tmp';
12+
$peerName = 'crypto-supplemental-read-timeout';
13+
14+
$serverCode = <<<'CODE'
15+
$ctx = stream_context_create(['ssl' => ['local_cert' => '%s']]);
16+
$flags = STREAM_SERVER_BIND|STREAM_SERVER_LISTEN;
17+
$server = stream_socket_server("tls://127.0.0.1:0", $errno, $errstr, $flags, $ctx);
18+
phpt_notify_server_start($server);
19+
20+
$conn = stream_socket_accept($server, 30);
21+
22+
fwrite($conn, "hello\n");
23+
24+
phpt_wait();
25+
fclose($conn);
26+
CODE;
27+
$serverCode = sprintf($serverCode, $certFile);
28+
29+
$clientCode = <<<'CODE'
30+
$ctx = stream_context_create(['ssl' => [
31+
'verify_peer' => false,
32+
'verify_peer_name' => false,
33+
'peer_name' => '%s',
34+
]]);
35+
36+
$client = stream_socket_client("tls://{{ ADDR }}", $errno, $errstr, 30, STREAM_CLIENT_CONNECT, $ctx);
37+
stream_set_blocking($client, true);
38+
stream_set_timeout($client, 5);
39+
$start = hrtime(true);
40+
41+
$buf = '';
42+
$read = [$client];
43+
$write = $except = null;
44+
while (true) {
45+
if (!stream_select($read, $write, $except, 5)) {
46+
break;
47+
}
48+
49+
// Initially, read only the first char, then request more than is stored
50+
// in the buffer, triggering a supplemental read.
51+
$chunk = fread($client, strlen($buf) === 0 ? 1 : 10);
52+
if ($chunk === '' || $chunk === false) {
53+
/* A non-application record (e.g. a TLS 1.3 session ticket) may arrive first. */
54+
if (feof($client)) {
55+
break;
56+
}
57+
} else {
58+
$buf .= $chunk;
59+
if (strlen($buf) >= 6) {
60+
break;
61+
}
62+
}
63+
$read = [$client];
64+
$write = $except = null;
65+
}
66+
67+
echo trim($buf), "\n";
68+
69+
$diff = (hrtime(true) - $start) / 1e9;
70+
var_dump($diff < 4.0);
71+
72+
phpt_notify();
73+
fclose($client);
74+
CODE;
75+
$clientCode = sprintf($clientCode, $peerName);
76+
77+
include 'CertificateGenerator.inc';
78+
$certificateGenerator = new CertificateGenerator();
79+
$certificateGenerator->saveNewCertAsFileWithKey($peerName, $certFile);
80+
81+
include 'ServerClientTestCase.inc';
82+
ServerClientTestCase::getInstance()->run($clientCode, $serverCode);
83+
?>
84+
--CLEAN--
85+
<?php
86+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'crypto_supplemental_read_timeout.pem.tmp');
87+
?>
88+
--EXPECT--
89+
hello
90+
bool(true)

ext/openssl/xp_ssl.c

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2069,7 +2069,11 @@ static ssize_t php_openssl_sockop_io(int read, php_stream *stream, char *buf, si
20692069

20702070
/* Only do this if SSL is active. */
20712071
if (sslsock->ssl_active) {
2072-
int retry = 1;
2072+
/* We have already returned some buffered data. Don't retry and don't
2073+
* block. We're just trying to fill the buffer more, but the stream might
2074+
* be empty, so we don't want to wait in vain. */
2075+
bool supplemental = stream->has_buffered_data;
2076+
int retry = !supplemental;
20732077
struct timeval start_time;
20742078
struct timeval *timeout = NULL;
20752079
int began_blocked = sslsock->s.is_blocked;
@@ -2082,11 +2086,11 @@ static ssize_t php_openssl_sockop_io(int read, php_stream *stream, char *buf, si
20822086
}
20832087

20842088
/* never use a timeout with non-blocking sockets */
2085-
if (began_blocked) {
2089+
if (began_blocked && !supplemental) {
20862090
timeout = &sslsock->s.timeout;
20872091
}
20882092

2089-
if (timeout) {
2093+
if (timeout || supplemental) {
20902094
php_openssl_set_blocking(sslsock, 0);
20912095
}
20922096

@@ -2160,7 +2164,7 @@ static ssize_t php_openssl_sockop_io(int read, php_stream *stream, char *buf, si
21602164
}
21612165

21622166
/* Don't loop indefinitely in non-blocking mode if no data is available */
2163-
if (began_blocked == 0) {
2167+
if (began_blocked == 0 || supplemental) {
21642168
break;
21652169
}
21662170

0 commit comments

Comments
 (0)