Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segmentation fault during request shutdown when memory limit is exceeded during closure initialization #12073

Closed
mszabo-wikia opened this issue Aug 29, 2023 · 2 comments

Comments

@mszabo-wikia
Copy link

mszabo-wikia commented Aug 29, 2023

Description

The following code:

<?php

class Wrapper {

	private $cb;

	public function __construct( callable $cb ) {
		$this->cb = $cb;
	}
}

class Dep {
	public function test() {
		echo "test";
	}
}

class ClosureTest {

	public function __construct(
		private Dep $dep,
		private Dep $dep2,
		private Dep $dep3,
		private Dep $dep4,
		private Dep $dep5,
		private Dep $dep6
	) {
	}

	private function output( $i, $k ) {
		echo $i . $k;
	}

	function create( int $i, int $k = 213 ) {
		return new Wrapper( function ( $j ) use ( $k ) {
			$this->output( $j, $k );

			$this->dep->test();
			$this->dep2->test();
			$this->dep3->test();
			$this->dep4->test();
			$this->dep5->test();
			$this->dep6->test();
		} );
	}
}

ini_set( 'memory_limit', '4875K' );

$test = new ClosureTest( new Dep(), new Dep(), new Dep(), new Dep(), new Dep(), new Dep() );
for ( $i = 0; $i < 10_000; $i++ ) {
	$$i = $test->create( $i );
}

reliably segfaults on PHP 8.2, 8.1. and 8.0 with opcache enabled (-dopcache.enable_cli=1).

This is a simplified reproducer—originally seen on PHP 8.0.30, in the FPM SAPI, for a request that hit its configured memory limit on a line of code instantiating a closure: https://github.com/wikimedia/mediawiki/blob/f071c22a9a3e7e399dcf3256c96a952f15291a69/includes/Revision/RevisionStore.php#L1786

There is no segmentation fault if Opcache is disabled.

Backtrace of that original error:

#0  zend_mm_free_heap (ptr=0x7631, heap=0x7ff6cb600040) at /usr/src/php/Zend/zend_alloc.c:1366
#1  _efree (ptr=0x7631) at /usr/src/php/Zend/zend_alloc.c:2551
#2  0x0000559226f12314 in destroy_op_array (op_array=0x7ff6639fc778) at /usr/src/php/Zend/zend_opcode.c:475
#3  0x0000559226f94ec9 in zend_closure_free_storage (object=0x7ff6639fc740) at /usr/src/php/Zend/zend_closures.c:490
#4  0x0000559226fa45aa in zend_objects_store_free_object_storage (objects=objects@entry=0x559227c220c8 <executor_globals+840>, 
    fast_shutdown=fast_shutdown@entry=true) at /usr/src/php/Zend/zend_objects_API.c:104
#5  0x0000559226f0df3f in shutdown_executor () at /usr/src/php/Zend/zend_execute_API.c:339
#6  0x0000559226f1d4a9 in zend_deactivate () at /usr/src/php/Zend/zend.c:1239
#7  0x0000559226eb8559 in php_request_shutdown (dummy=dummy@entry=0x0) at /usr/src/php/main/main.c:1854
#8  0x0000559226c42b8a in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/php/sapi/fpm/fpm/fpm_main.c:1942

PHP Version

PHP 8.2.9

Operating System

No response

@iluuu1994
Copy link
Member

Nice reproducer, thank you! I'm investigating this. The problem is likely that zend_create_closure_ex allocates memory before it writes to all the fields. At that point, the closure is already registered as an object, causing it to be freed by zend_closure_free_storage at shutdown. this function however assumes that all fields are correctly initialized. memsetting the closure might lead to an undesired performance hit. We may get away with zeroing just the relevant fields before allocating any memory.

iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Make sure we null/initialize/addref the relevant fields before allocating any
memory.

Fixes phpGH-12073
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Make sure we null/initialize/addref the relevant fields before allocating any
memory.

Fixes phpGH-12073
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Make sure we null/initialize/addref the relevant fields before allocating any
memory.

Fixes phpGH-12073
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Make sure we null/initialize/addref the relevant fields before allocating any
memory.

Fixes phpGH-12073
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Addref to relevant fields before allocating any memory. Also only set/remove the
ZEND_ACC_HEAP_RT_CACHE flag after allocating memory.

Fixes phpGH-12073
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Addref to relevant fields before allocating any memory. Also only set/remove the
ZEND_ACC_HEAP_RT_CACHE flag after allocating memory.

Fixes phpGH-12073
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Aug 29, 2023
Addref to relevant fields before allocating any memory. Also only set/remove the
ZEND_ACC_HEAP_RT_CACHE flag after allocating memory.

Fixes phpGH-12073
@mszabo-wikia
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants