Don't call handlers unregistered during wxEpollDispatcher::Dispatch() (PR #26930)
got3nks
Alternative to #26924, which resolved the handler through a descriptor map. @vadz preferred a wrapper pointer, which is what this does; both are on the table, so pick whichever you would rather have and close the other. Possibly related: #19118, the same wxWebSessionCURL teardown path, reported there as UnregisterFD() errors on an already-closed descriptor rather than as a memory-safety problem.
What is wrong
wxEpollDispatcher::Dispatch() stores the wxFDIOHandler pointer in epoll_event::data.ptr and calls through the copy epoll_wait() filled in before any handler ran:
for ( epoll_event *p = events; p < events + rc; p++ ) { wxFDIOHandler * const handler = (wxFDIOHandler *)(p->data.ptr); ... handler->OnReadWaiting();
Servicing one event of a batch can unregister the descriptor belonging to a later event of the same batch, and in real code destroy its handler with it. UnregisterFD() removes the descriptor from the kernel's epoll set, but nothing scrubs the pointer already copied into the array, so the loop goes on to make a virtual call on released memory.
wxSelectDispatcher is unaffected: ProcessSets() looks the handler up per ready descriptor.
What this does
epoll_event::data.ptr points at a small dispatcher-owned Entry holding the handler, rather than at the handler itself. Unregistering clears the handler in the entry, which is how Dispatch() tells that a registration is gone.
Entries are one per descriptor and reused across registrations. They are not freed on unregister, since a batch already in flight may still point at one, only when the dispatcher is destroyed.
Dispatch() never touches the entry container, reaching an entry through the pointer epoll_wait() returned, so the lock guarding growth of that container is never taken on the dispatching path nor held across a call into a handler.
A handler that is no longer registered is now skipped silently instead of asserting: after this change it is an expected outcome, not a programming error.
Test
tests/events/evtsource.cpp was an empty stub. It now contains a test that does not depend on timing: two pipes that are both readable are collected in one epoll_wait() batch, and whichever handler runs first unregisters the other, which must then not be called.
| events dispatched | unregistered handler | result | |
|---|---|---|---|
| before | 2 | called | 2 assertions fail |
| after | 1 | skipped | passes |
It also passes on wxSelectDispatcher, which already behaves this way.
Verified
Linux/aarch64, GCC 15.2, Ubuntu 26.04, ASan build of master, forced rebuild and relink on each side.
| reproducer, 200 runs | tests/events/evtsource.cpp |
|
|---|---|---|
| stock master | 0 pass, 200 heap-use-after-free | fails |
| this change | 200 pass, 0 failures | passes |
On stock, ASan reports the read at frame #0 in wxEpollDispatcher::Dispatch(), with the free one frame below it inside the other handler. The standalone reproducer is the one in #26924, unchanged.
Full test suite: 487 of 488 cases pass. The single failure, WebRequest::Sync::PostAfterRedirect, fails identically on unpatched master in this environment, so it is unrelated.
Not tested on x86_64.
Backport to 3.2
Still worth having there: no distribution ships 3.3, and the reports this came from are on 3.2.8 and 3.2.11. The map version backported to 3.2 with only cosmetic changes and passed 200/200 there; the wrapper needs the same treatment, and I will open that PR once you have settled on an approach, unless you would rather have it now.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26930
Commit Summary
- 8146b72 Don't call handlers unregistered during wxEpollDispatcher::Dispatch()
File Changes
(3 files)
- M include/wx/unix/private/epolldispatcher.h (32)
- M src/unix/epolldispatcher.cpp (52)
- M tests/events/evtsource.cpp (121)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26930.patch
- https://github.com/wxWidgets/wxWidgets/pull/26930.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
got3nks
@got3nks pushed 1 commit.
- 728adc5 Use a per-descriptor wrapper in wxEpollDispatcher instead of a map
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
got3nks
got3nks left a comment (wxWidgets/wxWidgets#26930)Rebased onto master now that ddd1e72 has landed, so this is no longer an alternative to the map version but a conversion of it to the wrapper you said you would prefer. Retitled and rewrote the body to match.
Net effect: Dispatch() no longer takes a lock per event, reaching the handler through the pointer epoll_wait() returned instead of looking it up. Your test passes unchanged, the reproducer is 200/200, and the full suite is 870/871 with the one failure reproducing identically on unpatched master here.
One point worth your call: Entry::handler ends up read without the lock in Dispatch(), which is the same cross-thread case your commit message already documents as racy. I can synchronise that read if you would rather.
No hurry on this, it is a cleanup and not a fix. The 3.2 backport is the part that still matters, and I will send whichever version you settle on here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 3 commits.
- 60bf248 squash! Use a per-descriptor wrapper in wxEpollDispatcher instead of a map
- da5327b squash! Use a per-descriptor wrapper in wxEpollDispatcher instead of a map
- 1fb3737 squash! Use a per-descriptor wrapper in wxEpollDispatcher instead of a map
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26930)Thanks, I think I do like the latest version better (do you agree or disagree?), but I couldn't help tweaking a few things, please let me know if you have any objections.
If not, I can (squash) merge this into master already and you can create a new PR with the commits which can be backported (I marked those that can't) and I'll merge it in 3.2 later.
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
got3nks
got3nks left a comment (wxWidgets/wxWidgets#26930)Agree, the latest version is better. On your head the test passes and the reproducer is 100/100.
Two issues in ForgetEntry(), both verified on 1fb3737:
-
resize(fd + 1)leaves the intermediate slots empty, so the range check passes butm_entries[fd]can still be null: unregistering a descriptor below the highest one seen that was never registered givesSEGV src/unix/epolldispatcher.cpp:142. I don't see an in-tree path that does it, but the map version tolerated it. -
The message asserts when it fires:
m_entries.size() - 1issize_tagainst%d, givingstrvararg.cpp(784): Format specifier mismatch for argument 2instead of the message. It also underflows toSIZE_MAXwhen the vector is empty.
Proposed fix: check the entry instead of the size, which drops the bad argument too:
wxCHECK_RET( fd < wxSsize(m_entries) && m_entries[fd], wxString::Format("Unregistering FD %d which was never registered", fd) ); m_entries[fd]->handler = nullptr;
Push it, or fold it in while squashing?
Backport: will do, 3.2 PR with the base commit plus the GetEntry() / ForgetEntry() split.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26930)Thanks for noticing this, we should indeed check for this in wxCHECK(). I'll add this and squash the commits and push it to master soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
got3nks
got3nks left a comment (wxWidgets/wxWidgets#26930)Thanks @vadz, will file the 3.2 PR once this one merges.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.