Prevent crash in Unix JoystickImpl with too many file descriptors

[vittorioromeo]

Description

Fixes #1900 by using the check @fvallee-bnx suggested. Note that hasMonitorEvent is used in isConnected here:

bool JoystickImpl::isConnected(unsigned int index)
{
    // See if we can skip scanning if udev monitor is available
    if (!udevMonitor)
    {
        // udev monitor is not available, perform a scan every query
        updatePluggedList();
    }
    else if (hasMonitorEvent())
    {
        // Check if new joysticks were added/removed since last update
        udev_device* udevDevice = udev_monitor_receive_device(udevMonitor);

        // If we can get the specific device, we check that,
        // otherwise just do a full scan if udevDevice == nullptr
        updatePluggedList(udevDevice);

        if (udevDevice)
            udev_device_unref(udevDevice);
    }

    if (index >= joystickList.size())
        return false;

    // Then check if the joystick is connected
    return joystickList[index].plugged;
}

joystickList[index].plugged might return the wrong value in that case, but it seems fine as it is an edge case (over 1024 file descriptors). Not sure if there's a nice way to avoid a hard limit on file descriptors, but this should at least solve the crash.

Tasks

• Tested on Linux
• Tested on Windows
• Tested on macOS
• Tested on iOS
• Tested on Android

How to test this PR?

It needs to be tested on Linux, I have not had a chance to do that yet.

[binary1248]

Not sure if there's a nice way to avoid a hard limit on file descriptors, but this should at least solve the crash.

I was thinking of just replacing select by poll. The limits of select have been known for a long time, which is why poll was designed to achieve the same functionality without the limitations.

I don't think we should jump to conclusions a bit too fast. We don't know what the user might be doing with their system. I can imagine for scientific/prosumer purposes that need some kind of visualisation SFML could have been used if it wasn't for this limitation.

https://man7.org/linux/man-pages/man2/select.2.html

WARNING: select() can monitor only file descriptors numbers that are less than FD_SETSIZE (1024)—an unreasonably low limit for many modern applications—and this limitation will not change. All modern applications should instead use poll(2) or epoll(7), which do not suffer this limitation.

[binary1248]

Something along the lines of:

#include <poll.h>

bool hasMonitorEvent()
{
    // This will not fail since we make sure udevMonitor is valid
    int monitorFd = udev_monitor_get_fd(udevMonitor);

    pollfd fds{ monitorFd, POLLIN, 0 };

    return (poll(&fds, 1, 0) > 0) && ((fds.revents & POLLIN) != 0);
}

[binary1248]

@vittorioromeo Would it be possible to fix this problem with the poll snippet I provided?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (2.6.x@9d28bf7). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head e7fabfd differs from pull request most recent head 3e7380f. Consider uploading reports for the commit 3e7380f to get more accurate results

@@           Coverage Diff           @@
##             2.6.x   #1941   +/-   ##
=======================================
  Coverage         ?   7.17%           
=======================================
  Files            ?     184           
  Lines            ?   15787           
  Branches         ?    4161           
=======================================
  Hits             ?    1132           
  Misses           ?   14523           
  Partials         ?     132           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d28bf7...3e7380f. Read the comment docs.

[binary1248]

@vittorioromeo I updated the commit to the poll snippet I posted above.

[eXpl0it3r]

@fvallee-bnx can you give this branch a try? Note that you may need to adjust more, since the master branch is undergoing major changes towards SFML 3

[binary1248]

This patch can also be backported to 2.6.x without issues.

[fvallee-bnx]

@eXpl0it3r looks good to me (patch backported to 2.5.3 - no crash). Thanks a lot!

[eXpl0it3r]

I've backported the change to SFML 2.6 then we can forward merge it onto master later on