Skip to content

Call _exit(2) not exit(3) on fatal non-recoverable errors #13699

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Call _exit(2) not exit(3) on fatal non-recoverable errors #13699

wants to merge 2 commits into from

Conversation

crrodriguez
Copy link
Contributor

On unrecoverable errors there is no point in trying to flush stdio buffers or run atexit handlers or perform any cleanup operation that may not even complete.
@crrodriguez
phpdbg: exit(3) cannot be used in signal handlers.
f2490b1 
Only as-safe functions can be called.
exit() flushes stdio buffers which is unsafe on this context
and might result in corruption.
call _exit instead.
@crrodriguez
Zend: use _exit(2) on OOM/memory/unrecoverable error conditions.
8daa1d9 
Running out of memory is not a normal process termination
go away quickly and avoid extra cleanup
that may not even complete under memory pressure/corruption
dstogov
dstogov reviewed Mar 18, 2024
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

I don't see a lot of sense in disabling libc cleanup code everywhere.
Do you have troubles with some particular error or with atexit() callbacks.
@crrodriguez
Copy link
Contributor Author

It is anyways the wrong api..if ZTS you need to put a lock on it..because older standards do not require exit to be thread safe..(current ones do however) You also don't want to call unknown exit handlers that may be put there by libraries linked to PHP.. at least not in abnormal termination..and maybe not even when exit success.
@dstogov
Copy link
Member

dstogov commented Mar 25, 2024

This worked for ages, and I don't remember any related bug reports.

@iluuu1994 @nielsdos @arnaud-lb could you please think about profitability, consequences and possible troubles.
Personally, I would prefer not to change this in case we don't have the real related bugs.
@nielsdos
Copy link
Member

There was once another PR that did this change but for an exit call within a signal handler. There it made sense because exit is not async-signal-safe whereas _exit is.
In this PR, I don't really see a benefit of doing this change. It may also prevent some legitimate library atexit calls, which may have unforeseen consequences. I agree this shouldn't be changed without a bug report.
@crrodriguez
Copy link
Contributor Author

ok, the i'll post the signal bits on a different PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Engine Extension: opcache SAPI: cgi SAPI: cli SAPI: fpm SAPI: phpdbg
3 participants
@crrodriguez @dstogov @nielsdos