From 573a56ccaf524caa97b338ccd854436eb86c250a Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Sun, 26 Jun 2022 15:34:33 +0100 Subject: [PATCH] Split SAPI deactivation to module and destroy parts Currently some SAPIs might bail out on deactivation. One of those SAPI is PHP-FPM that can bail out on request end if for example the connection is closed by the client (web sever). The problem is that in such case the resources are not freed and some values reset. The most visible impact can have not resetting the PG(headers_sent) which can cause erorrs in the next request. One such issue is described in #77780 bug which is also cover by a test in this commit. It seems reasonable to separate deactivation and destroying of the resource which means that the bail out will not impact it. --- main/SAPI.c | 12 ++++- main/SAPI.h | 2 + main/main.c | 6 ++- .../fpm/tests/bug77780-header-sent-error.phpt | 54 +++++++++++++++++++ sapi/fpm/tests/fcgi.inc | 25 ++++++--- sapi/fpm/tests/response.inc | 18 +++++-- sapi/fpm/tests/tester.inc | 18 +++++-- 7 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 sapi/fpm/tests/bug77780-header-sent-error.phpt diff --git a/main/SAPI.c b/main/SAPI.c index 0a7f219e847e5..eea417cedcc38 100644 --- a/main/SAPI.c +++ b/main/SAPI.c @@ -485,7 +485,7 @@ static void sapi_send_headers_free(void) } } -SAPI_API void sapi_deactivate(void) +SAPI_API void sapi_deactivate_module(void) { zend_llist_destroy(&SG(sapi_headers).headers); if (SG(request_info).request_body) { @@ -519,6 +519,10 @@ SAPI_API void sapi_deactivate(void) if (sapi_module.deactivate) { sapi_module.deactivate(); } +} + +SAPI_API void sapi_deactivate_destroy(void) +{ if (SG(rfc1867_uploaded_files)) { destroy_uploaded_files_hash(); } @@ -533,6 +537,12 @@ SAPI_API void sapi_deactivate(void) SG(global_request_time) = 0; } +SAPI_API void sapi_deactivate(void) +{ + sapi_deactivate_module(); + sapi_deactivate_destroy(); +} + SAPI_API void sapi_initialize_empty_request(void) { diff --git a/main/SAPI.h b/main/SAPI.h index b0d2928369fc0..0f9e896683a85 100644 --- a/main/SAPI.h +++ b/main/SAPI.h @@ -143,6 +143,8 @@ extern SAPI_API sapi_globals_struct sapi_globals; SAPI_API void sapi_startup(sapi_module_struct *sf); SAPI_API void sapi_shutdown(void); SAPI_API void sapi_activate(void); +SAPI_API void sapi_deactivate_module(void); +SAPI_API void sapi_deactivate_destroy(void); SAPI_API void sapi_deactivate(void); SAPI_API void sapi_initialize_empty_request(void); SAPI_API void sapi_add_request_header(const char *var, unsigned int var_len, char *val, unsigned int val_len, void *arg); diff --git a/main/main.c b/main/main.c index 9772d8fd760a6..40684f32dc146 100644 --- a/main/main.c +++ b/main/main.c @@ -1860,10 +1860,12 @@ void php_request_shutdown(void *dummy) zend_post_deactivate_modules(); } zend_end_try(); - /* 12. SAPI related shutdown (free stuff) */ + /* 12. SAPI related shutdown*/ zend_try { - sapi_deactivate(); + sapi_deactivate_module(); } zend_end_try(); + /* free SAPI stuff */ + sapi_deactivate_destroy(); /* 13. free virtual CWD memory */ virtual_cwd_deactivate(); diff --git a/sapi/fpm/tests/bug77780-header-sent-error.phpt b/sapi/fpm/tests/bug77780-header-sent-error.phpt new file mode 100644 index 0000000000000..b5e76918547f1 --- /dev/null +++ b/sapi/fpm/tests/bug77780-header-sent-error.phpt @@ -0,0 +1,54 @@ +--TEST-- +FPM: bug77780 - Headers already sent error incorrectly emitted +--SKIPIF-- + +--EXTENSIONS-- +session +--FILE-- +start(); +$tester->expectLogStartNotices(); +$tester + ->request( + headers: [ + 'PHP_VALUE' => "session.cookie_secure=1", + ], + readLimit: 10, + expectError: true + ); +$tester->request( + headers: [ + 'PHP_VALUE' => "session.cookie_secure=1", + ] + ) + ->expectNoError(); +$tester->terminate(); +$tester->close(); + +?> +Done +--EXPECT-- +Done +--CLEAN-- + diff --git a/sapi/fpm/tests/fcgi.inc b/sapi/fpm/tests/fcgi.inc index d17a659dd9c02..7d236c1b03a5e 100644 --- a/sapi/fpm/tests/fcgi.inc +++ b/sapi/fpm/tests/fcgi.inc @@ -26,6 +26,7 @@ namespace Adoy\FastCGI; class TimedOutException extends \Exception {} class ForbiddenException extends \Exception {} +class ReadLimitExceeded extends \Exception {} /** * Handles communication with a FastCGI application @@ -404,16 +405,24 @@ class Client /** * Read a FastCGI Packet * + * @param int $readLimit max content size * @return array + * @throws ReadLimitExceeded */ - private function readPacket() + private function readPacket($readLimit = -1) { if ($packet = fread($this->_sock, self::HEADER_LEN)) { $resp = $this->decodePacketHeader($packet); $resp['content'] = ''; if ($resp['contentLength']) { - $len = $resp['contentLength']; - while ($len && $buf=fread($this->_sock, $len)) { + $len = $resp['contentLength']; + if ($readLimit >= 0 && $len > $readLimit) { + // close connection so it can be re-set reset and throw an error + fclose($this->_sock); + $this->_sock = null; + throw new ReadLimitExceeded("Content has $len bytes but the limit is $readLimit bytes"); + } + while ($len && $buf = fread($this->_sock, $len)) { $len -= strlen($buf); $resp['content'] .= $buf; } @@ -473,15 +482,16 @@ class Client * * @param array $params Array of parameters * @param string $stdin Content + * @param int $readLimit [optional] the number of bytes to accept in a single packet or -1 if unlimited * @return array * @throws ForbiddenException * @throws TimedOutException * @throws \Exception */ - public function request_data(array $params, $stdin) + public function request_data(array $params, $stdin, $readLimit = -1) { $id = $this->async_request($params, $stdin); - return $this->wait_for_response_data($id); + return $this->wait_for_response_data($id, 0, $readLimit); } /** @@ -579,12 +589,13 @@ class Client * * @param int $requestId * @param int $timeoutMs [optional] the number of milliseconds to wait. + * @param int $readLimit [optional] the number of bytes to accept in a single packet or -1 if unlimited * @return array response data * @throws ForbiddenException * @throws TimedOutException * @throws \Exception */ - public function wait_for_response_data($requestId, $timeoutMs = 0) + public function wait_for_response_data($requestId, $timeoutMs = 0, $readLimit = -1) { if (!isset($this->_requests[$requestId])) { throw new \Exception('Invalid request id given'); @@ -608,7 +619,7 @@ class Client // but still not get the response requested $startTime = microtime(true); - while ($resp = $this->readPacket()) { + while ($resp = $this->readPacket($readLimit)) { if ($resp['type'] == self::STDOUT || $resp['type'] == self::STDERR) { if ($resp['type'] == self::STDERR) { $this->_requests[$resp['requestId']]['state'] = self::REQ_STATE_ERR; diff --git a/sapi/fpm/tests/response.inc b/sapi/fpm/tests/response.inc index b531feacdca7e..1d0783b66ec35 100644 --- a/sapi/fpm/tests/response.inc +++ b/sapi/fpm/tests/response.inc @@ -111,21 +111,31 @@ class Response } /** - * @param string $errorMessage + * @param string|null $errorMessage * @return Response */ public function expectError($errorMessage) { $errorData = $this->getErrorData(); if ($errorData !== $errorMessage) { - $this->error( - "The expected error message '$errorMessage' is not equal to returned error '$errorData'" - ); + $expectedErrorMessage = $errorMessage !== null + ? "The expected error message '$errorMessage' is not equal to returned error '$errorData'" + : "No error message expected but received '$errorData'"; + $this->error($expectedErrorMessage); } return $this; } + /** + * @param string $errorMessage + * @return Response + */ + public function expectNoError() + { + return $this->expectError(null); + } + /** * @param string $contentType * @return string|null diff --git a/sapi/fpm/tests/tester.inc b/sapi/fpm/tests/tester.inc index 155bc234a8b74..dce81bfeb76f9 100644 --- a/sapi/fpm/tests/tester.inc +++ b/sapi/fpm/tests/tester.inc @@ -593,6 +593,8 @@ class Tester * @param string|null $successMessage * @param string|null $errorMessage * @param bool $connKeepAlive + * @param bool $expectError + * @param int $readLimit * @return Response */ public function request( @@ -602,7 +604,9 @@ class Tester string $address = null, string $successMessage = null, string $errorMessage = null, - bool $connKeepAlive = false + bool $connKeepAlive = false, + bool $expectError = false, + int $readLimit = -1, ) { if ($this->hasError()) { return new Response(null, true); @@ -612,11 +616,17 @@ class Tester try { $this->response = new Response( - $this->getClient($address, $connKeepAlive)->request_data($params, false) + $this->getClient($address, $connKeepAlive)->request_data($params, false, $readLimit) ); - $this->message($successMessage); + if ($expectError) { + $this->error('Expected request error but the request was successful'); + } else { + $this->message($successMessage); + } } catch (\Exception $exception) { - if ($errorMessage === null) { + if ($expectError) { + $this->message($successMessage); + } elseif ($errorMessage === null) { $this->error("Request failed", $exception); } else { $this->message($errorMessage);