Fix proc_close() signal exit code handling

[alies-dev]

When a child process spawned via proc_open() is killed by a signal, proc_close() returns a raw/mangled waitpid status instead of a meaningful exit code. This makes it impossible to distinguish between "exited normally with code 11" and "killed by SIGSEGV".

The fix adds proper WIFSIGNALED()/WTERMSIG() handling in proc_open_rsrc_dtor() (which proc_close() calls internally), returning 128 + signal_number — the same convention used by bash and other shells.

proc_get_status() already handled this correctly via its signaled/termsig fields; only the proc_close() return value was broken.

Fixes GH-21292

---

The bug originally was found by using Psalm vimeo/psalm#11679 that uses composer/xdebug-handler: composer/xdebug-handler#161

[Girgias]

nit: eol before eof

[alies-dev]

Fixed, thanks!

This made me curious -- the .editorconfig already has insert_final_newline = true, but it's not enforced in CI or on pre-commit hook level. A simple check could catch this automatically. Two options that fit the repo's style:

1. Shell one-liner in CI (zero deps):
   git ls-files -z '*.phpt' '*.php' '*.c' '*.h' | xargs -0 -L1 sh -c 'test -z "$(tail -c1 "$0")" || echo "missing final newline: $0"'

2. editorconfig-checker — single Go binary, validates all .editorconfig rules at once. Used by several large projects (e.g., Kotlin).

Given how minimal the current CI tooling is, option 1 would probably be the least friction.

Please let me know what do you think -- I can create a PR

[Girgias]

I don't remember all the details, but at one point we tried to run Clang Format but it produces a large diff and apparently also altered the behaviour of code. But the first one is probably the most sensible as a base rule?
Although please provide the alternative when you open a PR as others might have a different opinion. :)

[bukka]

Seems like BC break to me...

[andypost]

does it need RFC?

[alies-dev]

@bukka yes, it does change behavior. So master only, not for 8.4/8.5. Rebased on master and added UPGRADING (BC section) and NEWS entries.

While today looking into it again I found one more case: if proc_get_status() reaps a signaled child first, proc_close() then gets ECHILD and returns -1. GH-10239 fixed exactly that for normal exits, but not for signals. So waitpid_cached() now caches the signaled status too. This also fixes repeated proc_get_status() calls, which reported signaled => false, termsig => 0 after the first one. cached becomes true for those children as well, hence the expectation changes in bug39322.phpt and proc_open02.phpt. Renamed the test to gh21292.phpt and added that case there.

pclose() has the same raw status decoding (plain_wrapper.c), and exec()/system() go through it too. I didn't touch it here since it would be one more BC change, but can do a follow-up if you think it makes sense.

@andypost

does it need RFC?

GH-10239 (#10250) also changed proc_close()'s return value on master during 8.3 development, with NEWS and UPGRADING entries and no RFC. Though that one fixed an error value (-1), and here a non-error value changes (e.g. SIGKILL: 9 -> 137), so it's not quite the same. The release policy says fixing clearly incorrect behavior "is not automatically considered a BC break", but the impact should be evaluated. I'd expect it to be small, since code relying on the old raw value can't tell a signal from a normal exit anyway, and proc_get_status() has signaled/termsig for anyone who needs to be exact (137 can still be a real exit(137)). Happy to post a heads-up on internals if you think it's worth it.