[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 #53821

Luc45 · 2024-02-07T03:44:43Z

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	N/A
License	MIT

This PR addresses a bug in Symfony's Process component affecting PHP versions prior to 8.3. In these versions, calling proc_get_status multiple times on the same process resource only returns the correct exit status on the first call, with subsequent calls returning -1 due to the process being discarded. This behavior can lead to race conditions and incorrect process status reporting in Symfony applications.

PHP 8.3 changelog notes/fix PR:

Executing proc_get_status() multiple times

Executing proc_get_status() multiple times will now always return the right value on POSIX systems.
Previously, only the first call of the function returned the right value. Executing proc_close()
after proc_get_status() will now also return the right exit code. Previously this would return -1.

Symfony Process is very good at avoiding this, but it's possible to trigger this behavior if you pass the Process as a reference to the output callback itself, and inside of it you invoke a method that eventually triggers proc_get_status, such as isRunning, isSuccessful, etc.

This PR tries to mimick PHP 8.3 behavior in older versions, caching the first reported valid exit status code once the process exits, and reusing the cached value if the new exit status code is now invalid.

I believe this bug has been pestering Symfony Process for a long time, as per this Stack Overflow question, but it was very hard to track it down, and only happens in certain conditions, and it also have a racing condition factor depending on the time between the last output of the process and it's termination.

Changes:

Added caching mechanism for the exit status in the Process component.
Ensured the cached status is used for subsequent proc_get_status calls if PHP version is below 8.3.

Proof of Concept:

To demonstrate the issue and the effectiveness of the fix, a proof of concept script is included below. This script uses Symfony's Process component to start a subprocess that outputs a message and exits with a specific status code. The script then attempts to retrieve the exit status of the process using getExitCode(). In PHP versions prior to 8.3, without the proposed fix, this script will often incorrectly report an exit code of -1 due to the race condition described earlier.

<?php

if ( getenv( 'OUTPUT' ) ) {
	#echo 'This should fail when "wait" gets large';
	sleep( 2 );
	echo 'This should fail even when "wait" is small';
	die( 123 );
}

use Symfony\Component\Process\Process;

require_once __DIR__ . '/vendor/autoload.php';

$wait = 0;

do {
	// Increasingly bigger waits.
	$wait += 100000;
	echo sprintf( 'Wait: %s', $wait ) . PHP_EOL;

	$p = new Process( [ 'php', __FILE__ ], __DIR__, [
		'OUTPUT' => 1,
	] );

	$p->start( function ( string $type, string $out ) use ( $p ) {
		echo $out . PHP_EOL;

		/**
		 * Calling most methods in Symfony Process that triggers
		 * updateStatus() can potentially trigger the -1 bug.
		 *
		 * @see Process::updateStatus()
		 */
		echo sprintf( 'Is Running: %s', $p->isRunning() ? 'Yes' : 'No' ) . PHP_EOL;
		echo sprintf( 'Exit Code: %s', $p->getExitCode() ) . PHP_EOL;
	} );

	while ( $p->isRunning() ) {
		usleep( $wait );
	}

	if ( ! $p->isSuccessful() ) {
		break;
	}
} while ( true );

$is_started = $p->isStarted();
$is_running = $p->isRunning();
$exit_code  = $p->getExitCode();

echo sprintf( 'Started: %s, Running: %s, Exit code: %s', $is_started, $is_running, $exit_code ) . PHP_EOL;

Impact:

This change only affects Symfony applications running on PHP versions below 8.3.
It improves the reliability of process status reporting in these environments.
No breaking changes or backward compatibility issues are introduced.

Testing:

I haven't added tests to this PR because:

This bug involves racing conditions, which is hard to replicate.
It's past 1 AM local time right now, and I've been debugging this for the past 6 hours.
The provided "Proof of Concept" can serve as an initial check to confirm the bug.

I'm really not the type of developer that does not write tests, but I beg my pardon for the reasons above.

Considerations

Based on the proc_open implementation, this fix might not be needed on Windows.
You can find additional context for this bug in this Stack Overflow answer.

carsonbot · 2024-02-07T03:44:49Z

Hey!

Thanks for your PR. You are targeting branch "5.4" but it seems your PR description refers to branch "5.4, 6.4, 7.0".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

nicolas-grekas

Can you please add a test case to cover the behavior?

src/Symfony/Component/Process/Process.php

nicolas-grekas · 2024-02-07T08:07:35Z

Reading now your full description... I'd still be great to figure out a test case :)

stof · 2024-02-07T12:12:37Z

A first testcase could be one running your reproducer (even if it does not fail 100% of the time due to relying on a race condition, it would still be likely to fail sometimes if we reintroduce the issue in the future)

src/Symfony/Component/Process/Process.php

Luc45 · 2024-02-07T14:24:45Z

I've added a test that reproduces the bug, it fails on 7.1 tag and passes on this branch.

Luc45 · 2024-02-07T14:43:53Z

If I remove the fix, you can see the racing condition factor in the tests causing the tests to fail erratically on the longRunning process test.

Luc45 · 2024-02-07T14:45:05Z

My tests uses sleep to mimick racing condition scenarios, which adds a total of 4 seconds to the Process test suite, which is not ideal. I've grouped them in time-sensitive, and I'm open to ideas. Maybe we could reduce the sleep from 2 to 1 second. [Done]

Luc45 · 2024-02-07T17:43:54Z

I don't know why CliDumperTest::testCollapse is failing, as it doesn't seem to use Process at all.

nicolas-grekas

LGTM thanks.

src/Symfony/Component/Process/Process.php

src/Symfony/Component/Process/Tests/ProcessTest.php

Luc45 · 2024-02-07T19:18:21Z

src/Symfony/Component/Process/Tests/ProcessTest.php

@@ -1556,7 +1602,6 @@ public function testNotTerminableInputPipe()

    /**
     * @param string|array $commandline
-     * @param mixed        $input


I didn't want to touch this but Fabbot code style tests asks to remove it. Bringing it up just for clarity.

Luc45 · 2024-02-08T14:31:37Z

7.2 tests fail because of the short closure syntax, want me to keep it @nicolas-grekas?

nicolas-grekas · 2024-02-08T14:38:01Z

nope, indeed

Luc45 · 2024-02-08T15:16:11Z

Tests in Windows is failing because it doesn't have sleep. I've tweaked it to use inline PHP with sleep instead, and I can confirm it works by prepending time to the command, adjusting the sleep in the spawning process and re-running the test.

Sleep 5: php ./phpunit symfony src/Symfony/Component/Process 83,90s user 59,06s system 688% cpu 20,776 total
Sleep 1: php ./phpunit symfony src/Symfony/Component/Process 80,13s user 58,70s system 1094% cpu 12,684 total

Since it runs a bunch of tests in parallel, we have to ignore 80s and 59s because that's CPU time, and look at "actual time" which is the last one: 20s and 12s respectively, which sounds about right. Re-running.

Luc45 · 2024-02-08T16:19:32Z

The remaining Windows failures seems unrelated

…sions Below 8.3

nicolas-grekas · 2024-02-09T09:36:00Z

Thank you @Luc45.

…-daubois) This PR was merged into the 5.4 branch. Discussion ---------- [Process] Fix failing tests causing segfaults | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix tests | License | MIT [This PR](#53821) introduces some code that [triggers segfaults](https://github.com/symfony/symfony/actions/runs/7870905388/job/21473073396) in newer branches (7.x especially). We can achieve the same goal by using some bash script which is lighter and avoid the segfaults. I first tried this fix on 7.x and solved the issue (that does not happen on older branches). Commits ------- 041e178 [Process] Fix failing tests causing segfaults

carsonbot added Status: Needs Review Bug labels Feb 7, 2024

carsonbot added this to the 5.4 milestone Feb 7, 2024

Luc45 mentioned this pull request Feb 7, 2024

Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 #53820

Closed

nicolas-grekas reviewed Feb 7, 2024

View reviewed changes

src/Symfony/Component/Process/Process.php Outdated Show resolved Hide resolved

nicolas-grekas added the Process label Feb 7, 2024

carsonbot changed the title ~~Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3~~ [Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 Feb 7, 2024

stof reviewed Feb 7, 2024

View reviewed changes

src/Symfony/Component/Process/Process.php Outdated Show resolved Hide resolved

Luc45 requested review from nicolas-grekas and stof February 7, 2024 14:28

nicolas-grekas approved these changes Feb 7, 2024

View reviewed changes

src/Symfony/Component/Process/Process.php Outdated Show resolved Hide resolved

src/Symfony/Component/Process/Tests/ProcessTest.php Outdated Show resolved Hide resolved

src/Symfony/Component/Process/Tests/ProcessTest.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 7, 2024

Luc45 commented Feb 7, 2024

View reviewed changes

Luc45 force-pushed the patch-2 branch from 174ee82 to ad96b23 Compare February 8, 2024 14:28

[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Ver…

500caa3

…sions Below 8.3

nicolas-grekas force-pushed the patch-2 branch from 2ef1b9b to 500caa3 Compare February 9, 2024 09:35

nicolas-grekas merged commit 3870d1e into symfony:5.4 Feb 9, 2024
10 of 11 checks passed

Luc45 mentioned this pull request Feb 9, 2024

Use dev version of symfony/process woocommerce/qit-cli#126

Merged

alexandre-daubois mentioned this pull request Feb 12, 2024

[Process] Fix failing tests causing segfaults #53909

Merged

This was referenced Feb 27, 2024

Release v5.4.36 #54074

Merged

Release v6.4.4 #54075

Merged

Release v7.0.4 #54076

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 #53821

[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 #53821

Luc45 commented Feb 7, 2024 •

edited

Loading

carsonbot commented Feb 7, 2024

nicolas-grekas left a comment

nicolas-grekas commented Feb 7, 2024

stof commented Feb 7, 2024

Luc45 commented Feb 7, 2024

Luc45 commented Feb 7, 2024

Luc45 commented Feb 7, 2024 •

edited

Loading

Luc45 commented Feb 7, 2024

nicolas-grekas left a comment

Luc45 Feb 7, 2024

Luc45 commented Feb 8, 2024

nicolas-grekas commented Feb 8, 2024

Luc45 commented Feb 8, 2024

Luc45 commented Feb 8, 2024

nicolas-grekas commented Feb 9, 2024

[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 #53821

[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 #53821

Conversation

Luc45 commented Feb 7, 2024 • edited Loading

Changes:

Proof of Concept:

Impact:

Testing:

Considerations

carsonbot commented Feb 7, 2024

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 7, 2024

stof commented Feb 7, 2024

Luc45 commented Feb 7, 2024

Luc45 commented Feb 7, 2024

Luc45 commented Feb 7, 2024 • edited Loading

Luc45 commented Feb 7, 2024

nicolas-grekas left a comment

Choose a reason for hiding this comment

Luc45 Feb 7, 2024

Choose a reason for hiding this comment

Luc45 commented Feb 8, 2024

nicolas-grekas commented Feb 8, 2024

Luc45 commented Feb 8, 2024

Luc45 commented Feb 8, 2024

nicolas-grekas commented Feb 9, 2024

Luc45 commented Feb 7, 2024 •

edited

Loading

Luc45 commented Feb 7, 2024 •

edited

Loading