Skip to content

fix: Check that the result of pcntl_waitpid is the PID of a managed process #1126

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NanoSector
Copy link

@NanoSector NanoSector commented Jun 6, 2025

Description

The --parallel option spawns processes with pcntl_fork and expects the result of pcntl_waitpid to be one of its spawned processes. It looks up the temporary output file of a process in the given $childProcs array without checking if the returned process ID is actually managed, which under some circumstances caused by external factors may result in an undefined array index error.

Suggested changelog entry

N/A, this change likely impacts a very small subset of users.

Related issues/external references

Fixes #1112

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
    • I looked for existing tests or infrastructure to integrate this change with (with the sample breaking command mentioned in the linked issue), but was unable to find any. I'm open to adding this command as a test in the appropriate place given some pointers.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@rodrigoprimo
Copy link
Contributor

I was able to reproduce the problem with the command that you provided in #1112 and confirm that this PR fixes it.

Here is the version of the command that I used:

bash -c 'bash --init-file <(echo "cd /path/to/phpcs/repo") -i -t -c "bin/phpcs --parallel=8 -p"'

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NanoSector Thank you for this PR and for your willingness to contribute to PHP_CodeSniffer!

While the bug being solved here is very much an edge-case and related to external factors, I also agree that a fix as straight-forward as this one should not be blocked because of that, so happy to merge this.

I looked for existing tests or infrastructure to integrate this change with (with the sample breaking command mentioned in the linked issue), but was unable to find any. I'm open to adding this command as a test in the appropriate place given some pointers.

This particular issue feels like a perfect example of the type of tests which could be run via BashUnit (Docs). Adding a BashUnit test set up to test the CLI commands has been on my "need to look into this" list for a while 😉

Would you be interested in having a look to see if you can create an initial test setup for PHPCS with BashUnit ?

Please note: there's no obligation to say "yes" as I also realize that creating an initial BashUnit set up may be more than you bargained for with this PR.
However, I definitely would like to safeguard this fix via automated tests, so if those won't be included in this PR, I'd appreciate it if you could open an issue as a reminder that a test for this particular issue still needs to be added.

@NanoSector
Copy link
Author

@jrfnl Thanks for your feedback!

BashUnit is something I hadn't heard of before but indeed sounds like a perfect fit to handle cases like these & to application test this package more globally.

I'd definitely be interested in looking into this and it seems straightforward enough, I might be able to take a look later today. This issue isn't blocking for me so I'll append this PR.

@jrfnl
Copy link
Member

jrfnl commented Jun 9, 2025

@NanoSector That's be amazing! If it helps, feel free to DM me on Mastodon if you want to talk things through in a call.

…rocess

The --parallel option spawns processes with pcntl_fork and expects the
result of pcntl_waitpid to be one of its spawned processes. It looks up
the temporary output file of a process in the given $childProcs array
without checking if the returned process ID is actually managed, which
under some circumstances caused by external factors may result in an
undefined array index error.

Fixes PHPCSStandards#1112
@NanoSector NanoSector force-pushed the bugfix/1112-check-waitpid-result branch 5 times, most recently from 6dd074a to 6d89e6d Compare June 9, 2025 09:48
@NanoSector
Copy link
Author

NanoSector commented Jun 9, 2025

Looks like something in the patch sequence for the unhappy flow test trips up the tests, disabling that fixes the pipeline - it works on my local machine however, but looking into it. Sorry for the spam you're probably getting.

@NanoSector NanoSector force-pushed the bugfix/1112-check-waitpid-result branch 5 times, most recently from 4732822 to 8df50e3 Compare June 9, 2025 11:36
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
@NanoSector NanoSector force-pushed the bugfix/1112-check-waitpid-result branch from 8df50e3 to 60b9ac1 Compare June 9, 2025 12:00
@NanoSector
Copy link
Author

@jrfnl I have implemented your feedback & implemented some basic bashunit tests. Few things of note:

  1. It uses an in-repository bashunit file, which seemed cleaner than relying on an external installer script.
  2. The unhappy flow tests use a temporary file in the current directory; I'd rather make them use a construction withmktemp as this would reduce the amount of code needed, but ran into issues where phpcbf would prepend the current working directory to the absolute path so it would fail trying to find the files.
  3. The tests for this specific bug are a bit messy... I ran into issues with macOS and its outdated bash version, updating it through Homebrew yielded more process control issues. I opted for a platform-dependent solution and have verified that both commands reproduce the error.
  4. I replaced the call to phpcs in the test.yml workflow file, it seemed to me that bashunit makes this obsolete.

Looking forward to your feedback!

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NanoSector Thank you for setting this up, amazing work!

I've looked the commit over. I really like that you've chosen the path of using test scripts. That will help with maintainability and gives contributors the ability to run these tests locally. 👍

I do have some feedback/questions.

  1. It uses an in-repository bashunit file, which seemed cleaner than relying on an external installer script.

I'm not so sure about that and I'm not a fan of having code from other projects committed into this repo. The risk of someone making changes to the script and breaking our upgrade path are non-negligable (as this is plain code and not something like a closed PHAR file).

So, I'd rather it be installed in the GH Actions workflow at a specific version and that a section be added to the CONTRIBUTING file on what these tests are/when to add tests to the BashUnit setup, and how to install (link to the BashUnit manual + example command) and run the tooling locally.

  1. The unhappy flow tests use a temporary file in the current directory; I'd rather make them use a construction with mktemp as this would reduce the amount of code needed, but ran into issues where phpcbf would prepend the current working directory to the absolute path so it would fail trying to find the files.

Are you talking about the copying of the Runner.php file and the patch you apply to that via tests/EndToEnd/patches/Runner.php_inject_style_error.patch ?

In my opinion, using real PHPCS files for the tests make the tests quite fragile.
Think: someone submits a PR which updates the Runner.php or the Ruleset.php file and the patch contains a CS error. Now suddenly the BashUnit tests would also be failing as the output expectations no longer match.
This could be quite confusing for contributors and can lead to time wasted debugging something which doesn't need debugging.

Would it be possible to run these tests on some simple test fixtures instead ? I believe that would also side-step the issue with the current working directory ?

So, instead of having a patches subdirectory, there would be a [Ff]ixtures subdirectory with some files which can be used in the tests.
Heads-up: You can find some example fixtures like that in the 4.x branch: https://github.com/PHPCSStandards/PHP_CodeSniffer/tree/4.x/tests/Core/Util/ExitCode/Fixtures/ExitCodeTest

And if you then run phpcbf in the tests with the --suffix=.fixed flag, the original test fixtures would not be changed by the test run (new files with a .fixed extension would be created instead and those can be .gitignored for that specific fixtures directory).

That should also remove the need for the set_up() method and the tear_down() method would then only need to remove the *.fixed files from the fixture directory (with the .gitignore being a fallback in case the tear_down() didn't run for whatever reason).

  1. The tests for this specific bug are a bit messy... I ran into issues with macOS and its outdated bash version, updating it through Homebrew yielded more process control issues. I opted for a platform-dependent solution and have verified that both commands reproduce the error.

🙌🏻

  1. I replaced the call to phpcs in the test.yml workflow file, it seemed to me that bashunit makes this obsolete.

For now, I'd like to keep that test step in place, if you don't mind. I don't mind some QA duplication. Better that, than the tests failing to find an issue before the code gets released.

Other than that, it looks like the tests aren't running/working on the Windows builds (example). The Linux jobs all show the testdox output, the Windows jobs don't show any output at all.

I'd expect the step to fail with a non-zero exit code if the tests couldn't run on Windows, preferably with some feedback on why it is failing.
This will probably need some debugging to figure out why the tests aren't running on Windows.
First thing I'd suggest to try, would be adding shell: bash to the step and see if that makes a difference.

Then, depending on whether the failure to run on Windows is related to PowerShell or due to an issue with BashUnit itself, you may also want to open an issue in the BashUnit repo about this.

If needs be, I'm okay with skipping these tests on Windows initially and we can look into fixing that at a later point in time.
Making these kind of tests compatible cross-OS will probably not be that straight-forward in a lot of cases, especially for the setup/teardown, so running these tests on Windows may make life a lot more difficult for maintaining these tests, though time will tell.

(Skipping on Windows would mean adding a skip condition to the step in GH Actions + opening an issue about this to be addressed later.)

Let me know what you think.

@@ -764,7 +764,7 @@ public function processFile($file)
* The reporting information returned by each child process is merged
* into the main reporter class.
*
* @param array $childProcs An array of child processes to wait for.
* @param array<non-negative-int, non-empty-string> $childProcs An array of child processes to wait for.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param array<non-negative-int, non-empty-string> $childProcs An array of child processes to wait for.
* @param array<int, string> $childProcs An array of child processes to wait for.

These annotation should not be tooling specific and non-negative-int and non-empty-string are PHPStan specific annotations.

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

Successfully merging this pull request may close these issues.

--parallel option fails if pcntl_waitpid returns non-managed process ID
3 participants