Skip to content

Fix #70198: Checking liveness does not work as expected #1456

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

Closed
wants to merge 1 commit into from
Closed

Fix #70198: Checking liveness does not work as expected #1456

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions main/streams/xp_socket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ static inline int sock_recvfrom(php_netstream_data_t *sock, char *buf, size_t bu

static int php_sockop_set_option(php_stream *stream, int option, int value, void *ptrparam)
{
int oldmode, flags;
int oldmode, flags, ret;
php_netstream_data_t *sock = (php_netstream_data_t*)stream->abstract;
php_stream_xport_param *xparam;

Expand Down Expand Up @@ -314,7 +314,11 @@ static int php_sockop_set_option(php_stream *stream, int option, int value, void
if (sock->socket == -1) {
alive = 0;
} else if (php_pollfd_for(sock->socket, PHP_POLLREADABLE|POLLPRI, &tv) > 0) {
if (0 >= recv(sock->socket, &buf, sizeof(buf), MSG_PEEK) && php_socket_errno() != EWOULDBLOCK) {
ret = recv(sock->socket, &buf, sizeof(buf), MSG_PEEK);
if (0 > ret && php_socket_errno() != EWOULDBLOCK && php_socket_errno() != EAGAIN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look correct at least on Windows where php_socket_errno() is a real function call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain more details ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the declaration of php_socket_errno(). If you've got an errno set, next successful function call will clear it. Therefore it needs to be saved before to check multiple error codes.

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 0 >= ret?

Copy link
Contributor

Choose a reason for hiding this comment

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

@staabm nope, it is the exact fix - 0 == ret means the counterpart did shutdown, no need to check errno as that means the stream is not alive anyway. errno only needs to be checked if ret < 0. However one could merge these two cases with || as both set alive = 0.

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weltling || could be used but I think it makes more clearly to separate it into two if statements. I would like to merge it if the php team prefers a merging style.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shafreeck yep, || was just a note, doesn't matter much. But please save the php_socket_errno() result before checking it twice, as possibly the second condition will not work as expected.

Thanks.

alive = 0;
}
if (0 == ret) {
alive = 0;
}
}
Expand Down