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
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
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: Sun Oct 27 16:01:27 2024 UTC