Skip to content

Commit 3503b1d

Browse files
bukkabukka
committed
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.
1 parent 986e731 commit 3503b1d

File tree

8 files changed

+119
-18
lines changed

8 files changed

+119
-18
lines changed

‎NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ PHP NEWS
1515
- FPM:
1616
. Fixed bug GH-8885 (FPM access.log with stderr begins to write logs to
1717
error_log after daemon reload). (Dmitry Menshikov)
18+
. Fixed bug #77780 ("Headers already sent..." when previous connection was
19+
aborted). (Jakub Zelenka)
1820

1921
- Streams:
2022
. Fixed bug GH-9316 ($http_response_header is wrong for long status line).

‎main/SAPI.c

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,7 @@ static void sapi_send_headers_free(void)
485485
}
486486
}
487487

488-
SAPI_API void sapi_deactivate(void)
488+
SAPI_API void sapi_deactivate_module(void)
489489
{
490490
zend_llist_destroy(&SG(sapi_headers).headers);
491491
if (SG(request_info).request_body) {
@@ -519,6 +519,10 @@ SAPI_API void sapi_deactivate(void)
519519
if (sapi_module.deactivate) {
520520
sapi_module.deactivate();
521521
}
522+
}
523+
524+
SAPI_API void sapi_deactivate_destroy(void)
525+
{
522526
if (SG(rfc1867_uploaded_files)) {
523527
destroy_uploaded_files_hash();
524528
}
@@ -533,6 +537,12 @@ SAPI_API void sapi_deactivate(void)
533537
SG(global_request_time) = 0;
534538
}
535539

540+
SAPI_API void sapi_deactivate(void)
541+
{
542+
sapi_deactivate_module();
543+
sapi_deactivate_destroy();
544+
}
545+
536546

537547
SAPI_API void sapi_initialize_empty_request(void)
538548
{

‎main/SAPI.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,8 @@ extern SAPI_API sapi_globals_struct sapi_globals;
143143
SAPI_API void sapi_startup(sapi_module_struct *sf);
144144
SAPI_API void sapi_shutdown(void);
145145
SAPI_API void sapi_activate(void);
146+
SAPI_API void sapi_deactivate_module(void);
147+
SAPI_API void sapi_deactivate_destroy(void);
146148
SAPI_API void sapi_deactivate(void);
147149
SAPI_API void sapi_initialize_empty_request(void);
148150
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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1860,10 +1860,12 @@ void php_request_shutdown(void *dummy)
18601860
zend_post_deactivate_modules();
18611861
} zend_end_try();
18621862

1863-
/* 12. SAPI related shutdown (free stuff) */
1863+
/* 12. SAPI related shutdown*/
18641864
zend_try {
1865-
sapi_deactivate();
1865+
sapi_deactivate_module();
18661866
} zend_end_try();
1867+
/* free SAPI stuff */
1868+
sapi_deactivate_destroy();
18671869

18681870
/* 13. free virtual CWD memory */
18691871
virtual_cwd_deactivate();

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
--TEST--
2+
FPM: bug77780 - Headers already sent error incorrectly emitted
3+
--SKIPIF--
4+
<?php include "skipif.inc"; ?>
5+
--EXTENSIONS--
6+
session
7+
--FILE--
8+
<?php
9+
10+
require_once "tester.inc";
11+
12+
$cfg = <<<EOT
13+
[global]
14+
error_log = {{FILE:LOG}}
15+
[unconfined]
16+
listen = {{ADDR}}
17+
pm = static
18+
pm.max_children = 1
19+
EOT;
20+
21+
$code = <<<EOT
22+
<?php
23+
echo str_repeat('asdfghjkl', 150000) . "\n";
24+
EOT;
25+
26+
$tester = new FPM\Tester($cfg, $code);
27+
$tester->start();
28+
$tester->expectLogStartNotices();
29+
$tester
30+
->request(
31+
headers: [
32+
'PHP_VALUE' => "session.cookie_secure=1",
33+
],
34+
readLimit: 10,
35+
expectError: true
36+
);
37+
$tester->request(
38+
headers: [
39+
'PHP_VALUE' => "session.cookie_secure=1",
40+
]
41+
)
42+
->expectNoError();
43+
$tester->terminate();
44+
$tester->close();
45+
46+
?>
47+
Done
48+
--EXPECT--
49+
Done
50+
--CLEAN--
51+
<?php
52+
require_once "tester.inc";
53+
FPM\Tester::clean();
54+
?>

‎sapi/fpm/tests/fcgi.inc

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ namespace Adoy\FastCGI;
2626

2727
class TimedOutException extends \Exception {}
2828
class ForbiddenException extends \Exception {}
29+
class ReadLimitExceeded extends \Exception {}
2930

3031
/**
3132
* Handles communication with a FastCGI application
@@ -404,16 +405,24 @@ class Client
404405
/**
405406
* Read a FastCGI Packet
406407
*
408+
* @param int $readLimit max content size
407409
* @return array
410+
* @throws ReadLimitExceeded
408411
*/
409-
private function readPacket()
412+
private function readPacket($readLimit = -1)
410413
{
411414
if ($packet = fread($this->_sock, self::HEADER_LEN)) {
412415
$resp = $this->decodePacketHeader($packet);
413416
$resp['content'] = '';
414417
if ($resp['contentLength']) {
415-
$len = $resp['contentLength'];
416-
while ($len && $buf=fread($this->_sock, $len)) {
418+
$len = $resp['contentLength'];
419+
if ($readLimit >= 0 && $len > $readLimit) {
420+
// close connection so it can be re-set reset and throw an error
421+
fclose($this->_sock);
422+
$this->_sock = null;
423+
throw new ReadLimitExceeded("Content has $len bytes but the limit is $readLimit bytes");
424+
}
425+
while ($len && $buf = fread($this->_sock, $len)) {
417426
$len -= strlen($buf);
418427
$resp['content'] .= $buf;
419428
}
@@ -473,15 +482,16 @@ class Client
473482
*
474483
* @param array $params Array of parameters
475484
* @param string $stdin Content
485+
* @param int $readLimit [optional] the number of bytes to accept in a single packet or -1 if unlimited
476486
* @return array
477487
* @throws ForbiddenException
478488
* @throws TimedOutException
479489
* @throws \Exception
480490
*/
481-
public function request_data(array $params, $stdin)
491+
public function request_data(array $params, $stdin, $readLimit = -1)
482492
{
483493
$id = $this->async_request($params, $stdin);
484-
return $this->wait_for_response_data($id);
494+
return $this->wait_for_response_data($id, 0, $readLimit);
485495
}
486496

487497
/**
@@ -579,12 +589,13 @@ class Client
579589
*
580590
* @param int $requestId
581591
* @param int $timeoutMs [optional] the number of milliseconds to wait.
592+
* @param int $readLimit [optional] the number of bytes to accept in a single packet or -1 if unlimited
582593
* @return array response data
583594
* @throws ForbiddenException
584595
* @throws TimedOutException
585596
* @throws \Exception
586597
*/
587-
public function wait_for_response_data($requestId, $timeoutMs = 0)
598+
public function wait_for_response_data($requestId, $timeoutMs = 0, $readLimit = -1)
588599
{
589600
if (!isset($this->_requests[$requestId])) {
590601
throw new \Exception('Invalid request id given');
@@ -608,7 +619,7 @@ class Client
608619
// but still not get the response requested
609620
$startTime = microtime(true);
610621

611-
while ($resp = $this->readPacket()) {
622+
while ($resp = $this->readPacket($readLimit)) {
612623
if ($resp['type'] == self::STDOUT || $resp['type'] == self::STDERR) {
613624
if ($resp['type'] == self::STDERR) {
614625
$this->_requests[$resp['requestId']]['state'] = self::REQ_STATE_ERR;

‎sapi/fpm/tests/response.inc

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -111,21 +111,31 @@ class Response
111111
}
112112

113113
/**
114-
* @param string $errorMessage
114+
* @param string|null $errorMessage
115115
* @return Response
116116
*/
117117
public function expectError($errorMessage)
118118
{
119119
$errorData = $this->getErrorData();
120120
if ($errorData !== $errorMessage) {
121-
$this->error(
122-
"The expected error message '$errorMessage' is not equal to returned error '$errorData'"
123-
);
121+
$expectedErrorMessage = $errorMessage !== null
122+
? "The expected error message '$errorMessage' is not equal to returned error '$errorData'"
123+
: "No error message expected but received '$errorData'";
124+
$this->error($expectedErrorMessage);
124125
}
125126

126127
return $this;
127128
}
128129

130+
/**
131+
* @param string $errorMessage
132+
* @return Response
133+
*/
134+
public function expectNoError()
135+
{
136+
return $this->expectError(null);
137+
}
138+
129139
/**
130140
* @param string $contentType
131141
* @return string|null

‎sapi/fpm/tests/tester.inc

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,8 @@ class Tester
593593
* @param string|null $successMessage
594594
* @param string|null $errorMessage
595595
* @param bool $connKeepAlive
596+
* @param bool $expectError
597+
* @param int $readLimit
596598
* @return Response
597599
*/
598600
public function request(
@@ -602,7 +604,9 @@ class Tester
602604
string $address = null,
603605
string $successMessage = null,
604606
string $errorMessage = null,
605-
bool $connKeepAlive = false
607+
bool $connKeepAlive = false,
608+
bool $expectError = false,
609+
int $readLimit = -1,
606610
) {
607611
if ($this->hasError()) {
608612
return new Response(null, true);
@@ -612,11 +616,17 @@ class Tester
612616

613617
try {
614618
$this->response = new Response(
615-
$this->getClient($address, $connKeepAlive)->request_data($params, false)
619+
$this->getClient($address, $connKeepAlive)->request_data($params, false, $readLimit)
616620
);
617-
$this->message($successMessage);
621+
if ($expectError) {
622+
$this->error('Expected request error but the request was successful');
623+
} else {
624+
$this->message($successMessage);
625+
}
618626
} catch (\Exception $exception) {
619-
if ($errorMessage === null) {
627+
if ($expectError) {
628+
$this->message($successMessage);
629+
} elseif ($errorMessage === null) {
620630
$this->error("Request failed", $exception);
621631
} else {
622632
$this->message($errorMessage);

0 commit comments

Comments
 (0)