Skip to content
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

Descriptors equal or higher than FD_SETSIZE can't be used with select() #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vnsavage
Copy link
Contributor

@vnsavage vnsavage commented Sep 7, 2022

As per select(2) manual page: Executing FD_CLR() or FD_SET() with a value of fd that is negative or is equal to or larger than FD_SETSIZE will result in undefined behavior.

If we don't check whether the file descriptor is less than FD_SETSIZE it's easy to trigger segfaults in the extension. This is easy to reproduce with the following script:

<?php

for ( $i = 0; $i < 1024; $i++ ) {
    socket_create_pair(AF_UNIX, SOCK_STREAM, 0, $socketpair);
    $sockets[] = $socketpair;
}

memcache_get(memcache_connect('memcache_host, 11211), 'some_key');

Long term the extension should probably be switched to a different polling API, but in the short term this patch will trigger Fatal Errors when we run out of allowed file descriptors so that segfaults are not generated.

@pmurzakov
Copy link
Contributor

pmurzakov commented Sep 8, 2022

I don't know how it is possible as this bug has been present at least since 16 years ago and never been reported and now we both found it at the same time. I also encountered it in our production system a few days ago and implemented alternative API based on poll() which doesn't have this limitation. (leaving select() as default option for compatibility)

here the PR #101

@vnsavage
Copy link
Contributor Author

Quite the coincidence indeed, switching to poll() sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants