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

Better validate serve_listeners handling of weird edge cases #492

Open
njsmith opened this issue Apr 13, 2018 · 1 comment
Open

Better validate serve_listeners handling of weird edge cases #492

njsmith opened this issue Apr 13, 2018 · 1 comment

Comments

@njsmith
Copy link
Member

njsmith commented Apr 13, 2018

Looking at twisted/twisted#996 led me to investigate more the difference between EMFILE and WSAEMFILE... the latter is the special "winsockets" version of EMFILE, and apparently they are actually different:

In [2]: errno.EMFILE
Out[2]: 24

In [3]: errno.WSAEMFILE
Out[3]: 10024

And right now serve_listeners has special handling for EMFILE, but not WSAEMFILE... so it's probably broken.

We should:

  • Add the WSA variants to the list of errnos here:
    ACCEPT_CAPACITY_ERRNOS = {
    errno.EMFILE,
    errno.ENFILE,
    errno.ENOMEM,
    errno.ENOBUFS,
    }
  • Double-check if we need to modify the list in SocketListener:
    _ignorable_accept_errno_names = [
    # Linux can do this when the a connection is denied by the firewall
    "EPERM",
    # BSDs with an early close/reset
    "ECONNABORTED",
    # All the other miscellany noted above -- may not happen in practice, but
    # whatever.
    "EPROTO",
    "ENETDOWN",
    "ENOPROTOOPT",
    "EHOSTDOWN",
    "ENONET",
    "EHOSTUNREACH",
    "EOPNOTSUPP",
    "ENETUNREACH",
    "ENOSR",
    "ESOCKTNOSUPPORT",
    "EPROTONOSUPPORT",
    "ETIMEDOUT",
    "ECONNRESET",
    ]
  • Modify our EMFILE test so that it actually provokes a real EMFILE, instead of a fake one like we do now:
    async def raise_EMFILE():
    raise OSError(errno.EMFILE, "out of file descriptors")
    listener.accept_hook = raise_EMFILE
@njsmith
Copy link
Member Author

njsmith commented Apr 14, 2018

On further investigation, this is not actually a real issue, because Windows doesn't actually have any way to limit the number of socket handles you allocate; if you do a while True: sockets.append(socket.socket()) then it doesn't return any kind of error, it just eventually locks up the whole system because no processes can get any handles.

Paste of some discussion on #twisted with @markrwilliams:

<runciter> njs: also, at some point i'd like to talk to you about WSAEMFILE on windows  
<runciter> though i can probably edit the trio issue you linked to that twisted PR
<runciter> err, comment on
<njs> runciter: sure
<njs> runciter: I don't really know anything about it yet though :-)
<runciter> njs: https://msdn.microsoft.com/en-us/library/windows/desktop/ms740522(v=vs.85).aspx
<runciter> njs: that's a citation, sort of, for why WSAEMFILE is different from EMFILE - sockets are generally not files
<runciter> as a consequence you can call os.dup(0) in a loop until you hit EMFILE, and then immediately allocate a socket
<runciter> calling socket.socket() in a loop hits some kind of limit, but it's not WSAEMFILE
<njs> does os.dup() even touch handles at all on windows?
<runciter> and it's an extraordinarily large number
<runciter> njs: unclear; https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/dup-dup2 is murky
<njs> on windows, handles are the real thing, and fds are this fake thing emulated in userspace by the CRT (= what they call libc). there's just a global array that maps from fd numbers to handles. So dup might just create a new entry in the array pointing to the same handle...
<runciter> also, https://github.com/twisted/twisted/pull/996#issuecomment-381213313
<runciter> libuv doesn't bother to do any EMFILE recovery on windows
<njs> (and for extra fun, the answer might be different for different versions of python, b/c they use different versions of the CRT)
<runciter> indeed :(
<njs> it looks like on 3.6 at least os.dup does create new handles:
<njs> https://paste.pound-python.org/show/Rm5xcGEzpZ0WXTd7hWNd/
<njs> runciter: the accept() docs do imply that WSAEMFILE is the way it reports out-of-handle errors, but ... I am certainly prepared to believe that they are lying
<runciter> but apparently _file_ handles, not socket handles
<runciter> njs: i am now pretty sure socket handles and file handles are very different things
<runciter> as evidenced by their different limits and the fact that there's EMFILE and WSAEMFILE
<njs> (also in general all the WSA error codes are different because of some weird history I don't understand where winsockets was like this add on API rather than something built into the OS, and it was originally a spec rather than an implementation, so they had to define the ABI separately from the regular windows ABI, or... soemthing? it is all extremely weird)
<runciter> njs: winsock 2 evidently lets you turn a socket handle into a file handle with some performance penalty (see that link) 
<runciter> err, the first msdn link
<runciter> it is super weird
<njs> runciter: I believe in modern windows, normally, socket handles are real OS handles 
<njs> "modern windows" and "normally" are doing a lot of work in that sentence
<runciter> hah
<runciter> njs: i'm not sure there's one kind of "handle" in windows
<runciter> truly, i'm not sure
<runciter> the MSDN docs for winsock 1 and 2 imply they aren't but they're not super clear
<njs> so there is a C type called HANDLE, which is an integer of a certain size
<njs> and this is punned to cover both OS-level handles and "pseudo-handles"
<runciter> hmmm
<njs> I think the hack is that there's a special bit that marks a HANDLE number as being a "pseudo-handle", which then gets special handling in user-space
<runciter> njs: do you have any helpful hints into MSDN docs on this?
<njs> so like the console handles are traditionally pseudo-handles
<runciter> or is this accumulated street smarts?
<njs> and winsock has its own thing, where traditionally SOCKETs are either a pointer to some user space emulation thing, or a real OS handle, and they've generally moved towards the latter over time (but both are still supported)
<njs> yeah, this is accumulated from reading many many little hints buried in different places unfortunately
<runciter> nuts, i see
<runciter> njs: so one possibility is that in my tests i was somehow using user space emulated handles
<runciter> my tests did confirm that os.dup(0) is not sufficient to trigger WSAEMFILE on a subsequent accept, though
<njs> I bet os.dup is hitting the fd limit before it hits the handle limit
<njs> hmm, I tried doing 'while True: socks.append(socket.socket())' to see what happened
<njs> and what happened is my terminal froze?
<njs> windows is so weird
<runciter> yes!
<runciter> right?
<runciter> there's https://msdn.microsoft.com/en-us/library/windows/desktop/ms737647(v=vs.85).aspx
<runciter> which i was going to use to try and figure out what was happening
<njs> haha wut
<njs> "The sign-in process couldn't display security and sign-in options when Ctrl+Alt+Delete was pressed. If Windows doesn't respond, press Esc, or use the power switch to restart."
<runciter> yup
<runciter> mine spontaenously shut down
<njs> so apparently the way windows reports out-of-handles errors is to blow up the world
<runciter> but then froze partway through
<njs> I like the crash-only philosophy but maybe this is taking it too far
<runciter> to be fair it's pretty good at getting attention
<njs> maybe this isn't anything we need to worry about then
<njs> the default handle limits are definitely much higher than the fd limits you'll usually see on unix
<runciter> njs: that seems to be libuv's take on it
<njs> "In one of the rare cases where Windows sets a hard-coded upper limit on a resource, the Executive defines 16,777,216 (16*1024*1024) as the maximum number of handles a process can allocate" -- https://blogs.technet.microsoft.com/markrussinovich/2009/09/29/pushing-the-limits-of-windows-handles/
<runciter> ...
<runciter> so, like
<njs> also: "The Microsoft Winsock provider limits the maximum number of sockets supported only by available memory on the local computer" -- https://msdn.microsoft.com/en-us/library/windows/desktop/ms739169(v=vs.85).aspx
<runciter> can you lower that?
<runciter> njs: there's a (drum roll) registry key supposedly
<njs> I'm not seeing any evidence of that, no
<runciter> but i wasn't able to find it on windows 10
<runciter> seems... bad?
<runciter> that might be unix brain damage
<njs> there's lots of references to a 10k limit, but that seems to be a limit on the number of window objects, not a general limit on handles

It would still be good to:

  • Double-check whether the list in SocketListener works correctly

  • Actually provoke an EMFILE during tests (on non-Windows platforms only)

@njsmith njsmith changed the title serve_listeners resource exhaustion handling is probably broken on Windows Better validate serve_listeners handling of weird edge cases Apr 14, 2018
@oremanj oremanj added the polish label May 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants