Skip to content

Fix GH-13860: Incorrect PHP_STREAM_OPTION_CHECK_LIVENESS case in ext/openssl/xp_ssl.c - causing use of dead socket #13895

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Apr 5, 2024

php_socket_errno() may return a stale value when recv returns a value >= 0. As such, the liveness check is wrong.
This is the same bug as #70198 (fixed in GH-1456). So we fix it in the same way.

…xt/openssl/xp_ssl.c - causing use of dead socket

php_socket_errno() may return a stale value when recv returns a
value >= 0. As such, the liveness check is wrong.
This is the same bug as #70198 (fixed in phpGH-1456). So we fix it in the
same way.
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Looks good to me. I also run the test and it is correct as well! Well done! ;)

int err;

ret = recv(sslsock->s.socket, &buf, sizeof(buf), MSG_PEEK|MSG_DONTWAIT);
err = php_socket_errno();
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need to always call it (e.g. when ret is positive) but it's not a big deal as it's just errno assignment on non Win or quick WSAGetLastError call on Win. I see you took it from xp_socket so it's fine to leave as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it probably doesn't matter too much in practice... I'll make a follow-up for master that changes this in xp_ssl and xp_socket, and we can see from there.

@nielsdos nielsdos closed this in 2aae14c Apr 7, 2024
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.

Incorrect PHP_STREAM_OPTION_CHECK_LIVENESS case in ext/openssl/xp_ssl.c - causing use of dead socket
3 participants