Skip to content

sapi/cli: Fix crash in the CLI server when a client is reset before being accepted - #23840

Closed
devnexen wants to merge 1 commit into
php:PHP-8.4from
devnexen:fix-cli-server-reset-crash
Closed

devnexen wants to merge 1 commit into
php:PHP-8.4from
devnexen:fix-cli-server-reset-crash

Conversation

@devnexen

Copy link
Copy Markdown
Member

accept() can return a socket without filling in the peer address when the client has already reset the connection. The address buffer was left uninitialized and php_network_populate_name_from_sockaddr() does not set the output string for an unknown family, so zend_string_dup() was handed a NULL.

Zero the buffer and fall back to "-" when the address is unknown.

…eing accepted

accept() can return a socket without filling in the peer address when the
client has already reset the connection.  The address buffer was left
uninitialized and php_network_populate_name_from_sockaddr() does not set the
output string for an unknown family, so zend_string_dup() was handed a NULL.

Zero the buffer and fall back to "-" when the address is unknown.
@devnexen
devnexen force-pushed the fix-cli-server-reset-crash branch from 5df65a1 to 103a931 Compare September 22, 2026 10:08
@devnexen devnexen changed the title Fix crash in the CLI server when a client is reset before being accepted sapi/cli: Fix crash in the CLI server when a client is reset before being accepted Sep 22, 2026
@devnexen
devnexen marked this pull request as ready for review September 22, 2026 10:51
@devnexen
devnexen requested a review from Girgias September 22, 2026 10:51

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM @Sjord ?

@Sjord

Sjord commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Looks good to me.

It seems to me that the test does not reliably test this, is that right? Even so, I don't think there's a better way to test this.

Does this need a NEWS entry?

@devnexen

Copy link
Copy Markdown
Member Author

there is a "follow-up" PR.

@devnexen devnexen closed this in 83c59b8 Sep 24, 2026
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.

3 participants