Fix bug #77780: "Headers already sent" when previous connection was aborted

This change primarily splits SAPI deactivation to module and destroy
parts. The reason is that 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 this fixes
and 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.

Author: bukka

NEWS

@@ -15,6 +15,8 @@ PHP                                                                        NEWS
 - FPM:
   . Fixed bug GH-8885 (FPM access.log with stderr begins to write logs to
     error_log after daemon reload). (Dmitry Menshikov)
+  . Fixed bug #77780 ("Headers already sent..." when previous connection was
+    aborted). (Jakub Zelenka)
 
 - Streams:
   . Fixed bug GH-9316 ($http_response_header is wrong for long status line).

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)
 {

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);

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();

sapi/fpm/tests/bug77780-header-sent-error.phpt

@@ -0,0 +1,54 @@
+--TEST--
+FPM: bug77780 - Headers already sent error incorrectly emitted
+--SKIPIF--
+<?php include "skipif.inc"; ?>
+--EXTENSIONS--
+session
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+[unconfined]
+listen = {{ADDR}}
+pm = static
+pm.max_children = 1
+EOT;
+
+$code = <<<EOT
+<?php
+echo str_repeat('asdfghjkl', 150000) . "\n";
+EOT;
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->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--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>

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;

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

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);