sapi/cli: support Expect 100-continue in PHP dev server

[Sjord]

When posting large payloads, curl checks whether the server is ready for the body. It sends an Expect: 100-continue header and expects HTTP/1.1 100 Continue as the response before sending the body. The PHP development server did not support this, causing a timeout in curl. This made such requests take one second longer.

• https://everything.curl.dev/http/post/expect100.html
• PHP development server does not support Expect 100-continue flow control #23242

[Sjord]

@php/release-managers-86 @mbeccati Is this something you want in PHP 8.6?

[mbeccati]

@Sjord Seems like a useful fix to me. I will discuss with the team.

EDIT: we agree it's perfectly fine for 8.6

[Sjord]

@iluuu1994 @Girgias Could you review this please?

[devnexen]

ok will try my best to review, I have some questions ...

[devnexen]

Is the dev server meant to exit when the peer aborts here? If I m not mistaken, php_cli_server_client_send_through() call is reached from a place where a failed send() is fatal to the whole process, right ?

[Sjord]

When reading the code I come to the same conclusion, but I cannot reproduce it. php_cli_server_client_send_through is also used in sapi_cli_server_send_headers, so I thought that this would be the appropriate function to use.

[devnexen]

right right ... note that sapi_cli_server_send_headers is wrapped in zend_try (and is a sapi handler) ; this is where I would like Gina opinion.

[Sjord]

I got a bit further with this.

• The test now sets the SO_LINGER option on the socket (if possible), which makes the send in php_cli_server_client_send_through fail. This indeed made the server exit.
• In the calling code php_cli_server_client_send_through is now wrapped within zend_try to handle the error.

So this seems solved, but I am also a little bit out of my depth here so it would be nice to get another set of eyes on this.

[devnexen]

Would it be possible to add this test ?

--TEST--
Failure to send "100 Continue" is reported with ignore_user_abort=1
--SKIPIF--
<?php
include "skipif.inc";
if (!extension_loaded("sockets")) die("skip sockets extension required");
if (PHP_OS_FAMILY === "Windows") die("skip SO_LINGER reset behaviour differs on Windows");
?>
--FILE--
<?php
$log = tempnam(sys_get_temp_dir(), 'cli_server_log');
$log_fd = fopen($log, 'ab');
$server = proc_open(
    [getenv('TEST_PHP_EXECUTABLE') ?: PHP_BINARY, '-n', '-d', 'ignore_user_abort=1', '-S', '127.0.0.1:0'],
    [0 => STDIN, 1 => $log_fd, 2 => $log_fd],
    $pipes,
    __DIR__
);

$port = null;
for ($i = 0; $i < 100 && $port === null; $i++) {
    usleep(50000);
    if (preg_match('@://127\.0\.0\.1:(\d+)\) started@', file_get_contents($log), $m)) {
        $port = $m[1];
    }
}

$fp = fsockopen('127.0.0.1', $port);
socket_set_option(socket_import_stream($fp), SOL_SOCKET, SO_LINGER, ['l_onoff' => 1, 'l_linger' => 0]);
fwrite($fp, "POST / HTTP/1.1\r\nExpect: 100-continue\r\nContent-Length: 4\r\n\r\n");
fclose($fp);

$output = '';
for ($i = 0; $i < 100 && !str_contains($output, 'Invalid request'); $i++) {
    usleep(50000);
    $output = file_get_contents($log);
}

var_dump(str_contains($output, 'Invalid request'), str_contains($output, 'Unexpected EOF'));

proc_terminate($server);
unlink($log);
?>
--EXPECT--
bool(true)
bool(false)

[devnexen]

so here it is send before the marlformed check below if I m not mistaken ?

[Sjord]

I swapped these two blocks. That indeed looks nicer and makes a little bit more sense.

[devnexen]

Almost there

 if (client->expect_continue && !client->request_read) {
              client->expect_continue = false;
              if (client->parser.http_major * 100 + client->parser.http_minor >= 101) {
                      /* RFC 9110 10.1.1: a 100-continue expectation in an HTTP/1.0 request must be
                       * ignored, so the interim response is only ever sent as HTTP/1.1. */
                      static const char continue_response[] = "HTTP/1.1 100 Continue\r\n\r\n";
                      bool send_success = false;
                      zend_try {
                              size_t sent = php_cli_server_client_send_through(client, continue_response, sizeof(continue_response) - 1);
                              send_success = sent == sizeof(continue_response) - 1;
                      } zend_end_try();
                      if (!send_success) {
                              *errstr = php_socket_strerror(php_socket_errno(), NULL, 0);
                              return -1;
                      }
              }
      }

[Sjord]

Thanks. I added the test and that led to quite a rabbit hole. But php_cli_server_client_send_through now returns the number of bytes sent and I check it in the calling code. I tried to make your test a bit nicer.

[devnexen]

I think it makes sense this get merged first and you rebase from it.

[devnexen]

Almost there

 if (client->expect_continue && !client->request_read) {
              client->expect_continue = false;
              if (client->parser.http_major * 100 + client->parser.http_minor >= 101) {
                      /* RFC 9110 10.1.1: a 100-continue expectation in an HTTP/1.0 request must be
                       * ignored, so the interim response is only ever sent as HTTP/1.1. */
                      static const char continue_response[] = "HTTP/1.1 100 Continue\r\n\r\n";
                      bool send_success = false;
                      zend_try {
                              size_t sent = php_cli_server_client_send_through(client, continue_response, sizeof(continue_response) - 1);
                              send_success = sent == sizeof(continue_response) - 1;
                      } zend_end_try();
                      if (!send_success) {
                              *errstr = php_socket_strerror(php_socket_errno(), NULL, 0);
                              return -1;
                      }
              }
      }