php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #60629 memory corruption when web server closed the fcgi fd(?)
Submitted: 2011-12-30 23:12 UTC Modified: 2012-01-03 22:26 UTC
From: phpbugs at oops dot mooo dot com Assigned: fat (profile)
Status: Closed Package: FPM related
PHP Version: 5.3.9RC4 OS: Debian Squeeze
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: phpbugs at oops dot mooo dot com
New email:
PHP Version: OS:

 

 [2011-12-30 23:12 UTC] phpbugs at oops dot mooo dot com
Description:
------------
I tried php5.3.9RC4 today and got a few core dumps.

I think b)fcgi_flush() returns false, making fcgi_write return -1. Then sapi_cgibin_single_write will make it positive because ret is unsigned in c). As a result, the comparison in d) will fail.

In debugger it looks like PHP is looping over open_packet() in an infinite loop. Each time out_pos gets increased a little. Finally, after overwriting the whole stack, it will SIGSEGV.

=== https://svn.php.net/repository/php/php-src/tags/php_5_3_9RC4/sapi/fpm/fpm/fastcgi.c ===
int fcgi_write(fcgi_request *req, fcgi_request_type type, const char *str, int len)
{
	int limit, rest;

	if (len <= 0) {
		return 0;
	}

	if (req->out_hdr && req->out_hdr->type != type) {
		close_packet(req);
	}

	/* Optimized version */
	limit = sizeof(req->out_buf) - (req->out_pos - req->out_buf);
	if (!req->out_hdr) {
		limit -= sizeof(fcgi_header);
		if (limit < 0) limit = 0;
	}

	if (len < limit) {
		if (!req->out_hdr) {
			open_packet(req, type);
		}
		memcpy(req->out_pos, str, len);
		req->out_pos += len;
	} else if (len - limit < sizeof(req->out_buf) - sizeof(fcgi_header)) {
		if (!req->out_hdr) {
a)			open_packet(req, type);
		}
		if (limit > 0) {
			memcpy(req->out_pos, str, limit);
			req->out_pos += limit;
		}
		if (!fcgi_flush(req, 0)) {
b)			return -1;
		}

=== https://svn.php.net/repository/php/php-src/tags/php_5_3_9RC4/sapi/fpm/fpm/fpm_main.c ===
static inline size_t sapi_cgibin_single_write(const char *str, uint str_length TSRMLS_DC)
{
c)	size_t ret;

	/* sapi has started which means everyhting must be send through fcgi */
	if (fpm_is_running) {
		fcgi_request *request = (fcgi_request*) SG(server_context);
		ret = fcgi_write(request, FCGI_STDOUT, str, str_length);
d)		if (ret <= 0) {
			return 0;
		}
		return ret;
	}


Patches

fpm-bugs-60629.patch (last revision 2012-01-03 18:02 UTC by fat@php.net)

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-12-30 23:40 UTC] phpbugs at oops dot mooo dot com
I think it might've been introduced in this commit (~line 270).

http://svn.php.net/viewvc/php/php-src/tags/php_5_3_9RC4/sapi/fpm/fpm/fpm_main.c?r1=317894&r2=317897

It looks like write() might have the same problem, since it returns a ssize_t that's casted to size_t.

Fix might be to use ssize_t instead?
 [2012-01-03 12:12 UTC] laruence@php.net
fat, could you plz look at this? thanks
 [2012-01-03 12:12 UTC] laruence@php.net
-Assigned To: +Assigned To: fat
 [2012-01-03 12:13 UTC] fat@php.net
it's on my todo list. I'll try to take time to look at this bugs this week.

++ jerome
 [2012-01-03 18:02 UTC] fat@php.net
The following patch has been added/updated:

Patch Name: fpm-bugs-60629.patch
Revision:   1325613774
URL:        https://bugs.php.net/patch-display.php?bug=60629&patch=fpm-bugs-60629.patch&revision=1325613774
 [2012-01-03 18:03 UTC] fat@php.net
Can you please test and validate the attached patch please ?

thx
++ jerome
 [2012-01-03 18:03 UTC] fat@php.net
-Status: Assigned +Status: Feedback
 [2012-01-03 19:44 UTC] phpbugs at oops dot mooo dot com
Looks good to me, I don't understand
a) Why was fcgi_write's return value changed to ssize_t
b) Why the explicit (size_t) casts was added
but I can't see any problem with them either :)

(I only did this part.)
-	size_t ret;
+	ssize_t ret
 [2012-01-03 22:25 UTC] fat@php.net
Automatic comment from SVN on behalf of fat
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=321734
Log: - Fixed bug #60629 (memory corruption when web server closed the fcgi fd)
 [2012-01-03 22:26 UTC] fat@php.net
-Status: Feedback +Status: Closed
 [2012-01-03 22:26 UTC] fat@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2012-04-18 09:46 UTC] laruence@php.net
Automatic comment on behalf of fat
Revision: http://git.php.net/?p=php-src.git;a=commit;h=0eb2eec938b705c864a711f5b0b80dc5cca48e26
Log: - Fixed bug #60629 (memory corruption when web server closed the fcgi fd)
 [2012-07-24 23:37 UTC] rasmus@php.net
Automatic comment on behalf of fat
Revision: http://git.php.net/?p=php-src.git;a=commit;h=0eb2eec938b705c864a711f5b0b80dc5cca48e26
Log: - Fixed bug #60629 (memory corruption when web server closed the fcgi fd)
 [2013-11-17 09:34 UTC] laruence@php.net
Automatic comment on behalf of fat
Revision: http://git.php.net/?p=php-src.git;a=commit;h=0eb2eec938b705c864a711f5b0b80dc5cca48e26
Log: - Fixed bug #60629 (memory corruption when web server closed the fcgi fd)
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Wed Dec 04 22:01:29 2024 UTC