gh-102494: fix MemoryError and data race in devpoll based selector on Solaris

[kulikjak]

When you set ulimit -n unlimited on Solaris and its derivatives (where /dev/poll is available) and import selectors, Python crashes with a MemoryError because there is no upper limit to the allocation size.

This fix adds an arbitrary limit of 2^18 (which results in roughly ~4MB of memory).

Fixes #102494

Edit: this also fixes the data race mentioned below: #102495 (comment)

• Issue: MemoryError and data race in devpoll selector on Solaris #102494

[arhadthedev]

@jcea (as a Solaris expert)

[jcea]

For reference, documentation at https://docs.oracle.com/cd/E19253-01/816-5177/6mbbc4g9n/index.html for example.

Notice that current array plays two roles:

1. Provide registration and removal of file descriptor information to the kernel. This operations don't require a huge array, you just flush descriptor information when the buffer is full or just right before the "poll" operation. This is already implemented in the "devpoll_flush()" function.

2. Get the result information from the "poll" operation. This requires being able to receive, in the worst case, all the registered file descriptors. Solaris will give back information up to "n" file descriptors if not enough space is available, but I don't know what would happen if these fds become active again. Maybe other file descriptors will starve, begging for service, but not receiving it because other active fds are notified and there is not enough space to receive all pending notifications at the same time (*).

Instead of allocating a huge array and clap it to a max size, I would suggest to keep a count of active file descriptors (registrations minus unregistrations) and resize the array as needed (maybe only growing it if needed, never shrinking it).

I would suggest an initial size of 1024, growing 25% when needed.

Shrinking would be nice too, but probably unimportant.

(*) Would be quite interesting to investigate the kernel implementation. I guess that playing with a handful of fds would be enough to determine if the kernel gives back the active fds in a round robin, random or "start from the beginning until you fill the buffer" way. Some comments at https://github.com/illumos/illumos-gate/blob/2c76d75129011c98e79463bb84917b828f922a11/usr/src/uts/common/io/devpoll.c#L237 suggest that Solaris kernel gives back the active descriptor in a round robin way, so actually using a small (1024 entries?) buffer would be fine enough. See also code around line 293 and 629. This assumption needs to be tested, but the intention seems quite clear.

If this assumption holds true and you want a highly concurrent/performant implementation, you could resize the array when the returned list of active file descriptors use the full array, signaling that a bigger array could reduce syscall traffic, although if you are dealing with Python, this kind of optimizations are probably overkill.

PS: I would support a "port" interface, although Solaris is not a tier-1 platform nowadays for Python.

[kulikjak]

Thank you for the detailed notes; I will look into it.

I've also recently hit another issue with the Devpoll selector when working on Cheroot (cherrypy/cheroot#561), so it needs some love.

[serhiy-storchaka]

Any progress? I support @jcea's idea.

[kulikjak]

Uh, I am sorry - this one's gone missing from my todo list...

Thanks for the review and extensive notes. Originally, I was under the assumption (I think - it's quite some time since I filled this) that we need as big of an array as is the number of watched descriptors, but we indeed don't.

I played with devpoll a little and the active file descriptors are indeed returned in a round robin way (at least on Oracle Solaris), so we don't need to worry about starvation.

I wonder whether we even need to resize the array. The performance would probably be slightly better in cases where devpoll.poll hits the upper limit often, but then we would likely need shrinking as well, and we probably don't want to expand/shrink based on a single devpoll.poll so we would need to watch n previous polls for the number of returned descriptors and make the decision to expand or shrink based on some average. That said, it can certainly be done.

We can also provide a new method to resize the buffer - most people would likely be happy with 1024, and those with a huge number of expected active descriptors can control the size of the returned tuple themselves.

[kulikjak]

And as for the port interface, I'll keep that in mind. It certainly doesn't have the highest priority, and thus I don't know when I'll have some time to look into that, but it would be a nice addition. Thanks!

[kulikjak]

So, I found the root cause of the problem I wrote about above (cherrypy/cheroot#561), and coincidentally it's relevant here.

In my testing, I saw internal_devpoll_register being called with one fd, but subsequent devpoll_flush registering a different one.

It happens when select_devpoll_poll_impl is called in one thread, then here:

        Py_BEGIN_ALLOW_THREADS
        errno = 0;
        poll_result = ioctl(self->fd_devpoll, DP_POLL, &dvp);
        Py_END_ALLOW_THREADS

right after Py_BEGIN_ALLOW_THREADS, a different thread calls internal_devpoll_register with a different descriptor, which is immediatelly overwritten with the result from the ioctl call. (I believe this can also happen in reverse - ioctl returning a descriptor, and before it reaches Py_END_ALLOW_THREADS, another thread overwrites it with one that is yet to be registered.)

Forcing a devpoll_flush after every register/unregister call seems to fix the issue, though that might still not be correct in the ioctl first, and register right after that case.

Because of that, I split the buffer into two - one for polling and one for registering/unregistering.

The one for polling now has a limit of min(limit.rlim_cur, 1024) - as mentioned in the existing comment "If we try to process more than getrlimit() fds, the kernel will give an error", which is why the limit.rlim_cur needs to be there. As I mentioned above, I am unsure whether we even need to have this buffer resizable - if so, I have a possible implementation with getter/setter for the maximum buffer size here: kulikjak@066314e

The register/unregister buffer has no specific requirements and will work no matter the size (128 seems like a good size ;)).

Let me know what you think. Thanks!

[encukou]

@kulikjak @jcea This is Solaris-specific code; happy to merge it if you approve.

[serhiy-storchaka]

Can we have any tests? Is this issue reproducible on OpenIndiana?

[kulikjak]

Can we have any tests? Is this issue reproducible on OpenIndiana?

Devpoll is tested with all the selector tests from test_selectors.py.

If you mean a test for this issue (having one buffer being used by two different operations), I don't know how feasible that is.

As for OpenIndiana, I didn't test that, but they are using the same patch as we do:
https://github.com/OpenIndiana/oi-userland/blob/b8dbc1a26f407f1611e2d0487e2c67f11b92ec93/components/python/python-314/patches/20-selectmodule.patch
so my guess is yes.

[kulikjak]

I just realized that this PR is no longer about just the MemoryError (how it started), but also includes the "two buffers" change, which is pretty unrelated. I wonder whether I should split it into separate issues/PRs?

[kulikjak]

One more thing - since devpoll select is not directory affected by RLIMIT_NOFILE/FD_SETSIZE (because the select limit is currently hardcoded to 1024), the test_above_fd_setsize from ScalableSelectorMixIn fails (unless RLIMIT_NOFILE is also 1024 or less). Therefore, I removed it from DevpollSelector tests.

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[kulikjak]

This is still relevant and I believe ready for review and push. We are already using this patch internally:
https://github.com/oracle/solaris-userland/blob/master/components/python/python313/patches/22-selectmodule.patch

[kulikjak]

@jcea, would you please check the changes so we can merge it? Thanks!

[vstinner]

When you set ulimit -n unlimited on Solaris and its derivatives (where /dev/poll is available) and import selectors, Python crashes with a MemoryError because there is no upper limit to the allocation size.
This fix adds an arbitrary limit of 2^18 (which results in roughly ~4MB of memory).

I agree that allocating an "unlimited" amount of memory is a bad idea :-)

But I'm not sure that the change is correct. I wrote the following script:

import socket, select

#SIZE = 5 
SIZE = 1050

read_fds = []
write_fds = []
for _ in range(SIZE):
    s1, s2 = socket.socketpair()
    read_fds.append(s1)
    write_fds.append(s2)

p = select.devpoll()
for fd in read_fds:
    p.register(fd, select.POLLIN)
#for fd in write_fds:
#    p.register(fd, select.POLLOUT)

for sock in write_fds:
    sock.send(b'abc')
print(f"expect {len(write_fds)} events")

events = p.poll()
print(f"got {len(events)} events")

for sock in read_fds:
    sock.close()
for sock in write_fds:
    sock.close()

Output with this change on OpenIndiana:

expect 1050 events
got 1024 events

It seems like devpoll.poll() is now limited to 1024 events, even if there are more events available, and so some events are missed :-(

I'm not sure that it's correct to allocate a fixed buffer of 1024 entries for "out fds". devpoll.register() should reallocate the buffer to the total number of registered file descriptors.

[kulikjak]

That is indeed the case. The limit is currently 1024 events, but the rest is not forgotten or thrown away - if you process some of them, you can get the rest.

The following added to your script:

....
events = p.poll()
print(f"got {len(events)} events")

for i in range(100):
    read_fds[i].recv(100)

events = p.poll()
print(f"got {len(events)} events")

results in:

expect 1050 events
got 1024 events
got 950 events

My thinking here was that the application handling events is likely running in a loop of "poll and handle all returned" and the number of returned events shouldn't break that (it will just poll more often, but each event will still be handled at some point). But thinking about it now, it's true that if the program wants to e.g. do some priority sorting, that will break things.

I see that the other selectors are apparently not doing this. With /dev/poll, there unfortunately isn't a way to simply ask the OS to return allocated buffer with all events because I am the one supplying the buffer of certain size. Do you suggest maybe counting each register call (and subtracting unregister calls) and making sure that the buffer is at least as big when poll is called? That should work.

[kulikjak]

This works as long as the user won't start calling unregister on non-registered descriptors - then the counter might get pretty wrong.

My other idea is to run the poll and if full array is returned, realloc it to a bigger one and run it again (and do so until you get all the fds). I don't know how the performance of that look like, but I think that the reallocation would happen very sparingly.

[vstinner]

The change now mostly LGTM. I just have a little concern about the getrlimit(RLIMIT_NOFILE) call in the constructor. Is it really useful to call it?

[vstinner]

Is it really worth it to getrlimit(RLIMIT_NOFILE) if we limit the value to 128 anyway? I suggest to always use 128.

select.devpoll.poll() enlarges outsize on demand anyway.

[vstinner]

Oh, the "Change detection / Create context from changed files" CI failed with "fatal: shallow file has changed since we read it". That's the issue gh-151365. I can fix it later.

[vstinner]

@kulikjak: I pushed changes to your branch. Would you mind to review them?

---

My thinking here was that the application handling events is likely running in a loop of "poll and handle all returned" and the number of returned events shouldn't break that (it will just poll more often, but each event will still be handled at some point). But thinking about it now, it's true that if the program wants to e.g. do some priority sorting, that will break things.

My worry is that maybe the first 1024 fds will "always" be ready, and so events on the following file descriptors may never be reached :-( It can introduce high latency which would depend on the file descriptor number, not good.

I see that the other selectors are apparently not doing this. With /dev/poll, there unfortunately isn't a way to simply ask the OS to return allocated buffer with all events because I am the one supplying the buffer of certain size.

I started to write a patch for your PR, but then I noticed that you already wrote a fix 1 hour ago, great!

This works as long as the user won't start calling unregister on non-registered descriptors - then the counter might get pretty wrong.

Hum, your registered++ and registered-- approach looks fragile. It doesn't take in account that the same file descriptor can be registered twice by devpoll.register(fd). And as you wrote, the counter gets wrong if you unregister the same file descriptor twice.

So I took the liberty of pushing a fix: I added a set object to keep track of the exact number of registered file descriptors. In short, devpoll.register(fd) calls registered.add(fd) and devpoll.unregister(fd) calls registered.discard(fd).

While testing my change for reference leak using ./python -m test -R 3:3 test_devpoll, I noticed leaks of file descriptors. But in fact, the leak is already in the current main branch.

I pushed a second change to fix test_devpoll leaks.

I also changed the NEWS entry to add a link to select.devpoll().

[vstinner]

I clicked on [Update branch] to fix the "fatal: shallow file has changed since we read it" error seen on the "Change detection / Create context from changed files" CI.

[serhiy-storchaka]

I have concerns about re-entrancy. Any allocations in register() can start a GC pass, which can run arbitrary Python via finalizers -- including something that calls close() on this very selector.

[serhiy-storchaka]

This looks very suspicious. On failure, it leaks memory and leave the object in inconsistent state.