Don't call handlers unregistered during wxEpollDispatcher::Dispatch() (PR #26924)
got3nks
wxEpollDispatcher::Dispatch() stores the wxFDIOHandler pointer in epoll_event::data.ptr and calls through the copy that 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 has never had this problem: ProcessSets() calls FindHandler(fd) for each ready descriptor and skips the ones that are gone. This makes the epoll dispatcher do the same — deriving it from wxMappedFDIODispatcher, which already keeps the fd-to-handler map and provides that lookup, storing the descriptor in epoll_event::data.fd, and resolving the handler when the event is actually processed.
A handler that is no longer registered is skipped silently rather than asserted on: after this change that is an expected outcome of the case above, 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.
How it was found
Investigating a crash in aMule, where wxWebRequest's curl backend destroys transfer event sources from inside curl callbacks that themselves run during Dispatch(). The investigation and a real-world reproducer built on wxFileSystemWatcher are @ngosang's, in amule-org/amule#1136. Under ASan that reproducer faulted in wxEpollDispatcher::Dispatch() on 4 of 5 runs before this change and 0 of 8 after.
wxWidgets #19118 looks like the same teardown ordering seen from the logging side — wxWebSessionCURL unregistering a descriptor curl has already closed.
Verified
Built and tested on Linux/aarch64 against master. wx-config --version 3.3.4, GCC 15.2, Ubuntu 26.04. Compiles warning-free under the project's own flags, full wxBase builds cleanly, and the before/after above was produced with a forced rebuild on each side.
I have not tested this on x86_64.
Please consider backporting to 3.2
This crashes released builds, and 3.2 is the series users actually get. wxWidgets' own downloads page lists 3.3.3 as the Development Release and 3.2.11 as the Latest Stable Release, and distributions have followed that:
| wx | |
|---|---|
| Debian 13 trixie / 14 forky / sid | 3.2.8 / 3.2.11 / 3.2.11 |
| Ubuntu 24.04 LTS / 25.10 / 26.04 | 3.2.4 / 3.2.9 / 3.2.11 |
| Arch (rolling), Fedora Rawhide | 3.2.11, 3.2.11 |
| Alpine edge, Gentoo, FreeBSD ports | 3.2.9, 3.2.8.1, 3.2.8.1 |
No Linux distribution ships 3.3, and Debian has no libwxgtk3.3 package in any suite. A fix that lands only on master therefore reaches approximately none of the affected users, and the reports it came from are on 3.2: the original crash is wx 3.2.8, and four independent cores were collected on 3.2.11.
I checked the 3.2 branch: Dispatch() there has the identical unguarded loop, and wxMappedFDIODispatcher with FindHandler() is already present, so the same approach works. This diff does not apply as-is — 3.2 predates the nullptr modernisation, so the context differs — but the adaptation is mechanical, and I am happy to prepare and test a 3.2 patch if that would help.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26924
Commit Summary
- 811595a Don't call handlers unregistered during wxEpollDispatcher::Dispatch()
File Changes
(3 files)
- M include/wx/unix/private/epolldispatcher.h (2)
- M src/unix/epolldispatcher.cpp (38)
- M tests/events/evtsource.cpp (130)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26924.patch
- https://github.com/wxWidgets/wxWidgets/pull/26924.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 left a comment (wxWidgets/wxWidgets#26924)Converting this to draft — CI found a real defect in the patch, and it needs rework rather than a touch-up.
Ubuntu 26.04 wxGTK UTF-8 fails in the socket tests:
wxWidgets assert: i != m_handlers.end() failed at src/common/fdiodispatcher.cpp:111
in ModifyFD with message 'modifying unregistered handler?' in a worker thread.
[01] wxMappedFDIODispatcher::ModifyFD
[02] wxEpollDispatcher::ModifyFD
[03] wxFDIOManagerUnix::AddInput
[04] wxSocketFDBasedManager::Install_Callback
[07] wxSocketImpl::Accept
[10] SocketServerThread::Entry
Two problems, both mine:
1. Two sources of truth. wxFDIOManagerUnix::AddInput() chooses between RegisterFD and ModifyFD from handler->GetRegisteredEvents() — state held on the handler — while wxMappedFDIODispatcher::ModifyFD asserts against the dispatcher's own map. Those can disagree, and a handler carrying a non-zero mask can reach ModifyFD for an fd the map has never seen. That was previously harmless because the epoll ModifyFD was stateless: epoll_ctl(EPOLL_CTL_MOD) on an unknown descriptor returns ENOENT and logs. Deriving from wxMappedFDIODispatcher turned a tolerated inconsistency into an assertion.
2. Thread safety. The assert fires in a worker thread, and that is the more serious issue. epoll_ctl() is kernel-serialised, so the previous implementation was effectively lock-free; std::map is not. The socket code registers and modifies descriptors from multiple threads, so the patch introduces an unsynchronised shared map into a path that did not have one. The socketStream/Output_PutC SIGSEGV in the same run is consistent with that.
For what it's worth, this suggests wxSelectDispatcher may carry the same latent inconsistency around ModifyFD, since it has always had that map — it simply is not exercised on Linux, where epoll is the default.
The underlying bug is still real, and the regression test in this PR still demonstrates it (before: both handlers run; after: only the surviving one). But Dispatch() needs an fd-to-handler lookup — epoll_event.data is a union, so storing the descriptor loses the handler pointer — and any such lookup has to avoid the asserting ModifyFD contract and be safe against concurrent registration. That is a bigger change than making epoll match select, and I would rather get it right than iterate in your CI.
My mistake in validating this: I exercised the dispatcher through pipes and wxFileSystemWatcher only, never through sockets, which is where your own test suite found it immediately. I will rework it and run the full test suite locally before marking this ready again.
Apologies for the noise.
—
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.
- 44fc600 Don't call handlers unregistered during wxEpollDispatcher::Dispatch()
—
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#26924)Reworked and pushed; CI is green.
This revision keeps its own map inside wxEpollDispatcher, guarded by a critical section, and records handlers unconditionally rather than deriving from wxMappedFDIODispatcher — the comment above has why that first approach failed. One correction to what I said there: a local full-suite run is not a sufficient gate for this, since it passes on my machine (aarch64, --disable-gui) even with the broken revision. CI is what caught it.
Verification, re-measured against the code in this PR:
- Standalone reproducer (in the description): 200 runs each way against an ASan-instrumented wx. Pre-fix, all 200 report
heap-use-after-freewithwxEpollDispatcher::Dispatch()as the faulting frame; post-fix, 0/200. Deterministic — no timing dependence. tests/events/evtsource.cpp, pre-fix:CHECK( numEvents == 1 )fails with expansion2 == 1, andCHECK( handler1.m_called != handler2.m_called )withtrue != true— both handlers ran. Post-fix: 7 assertions, all passing.- Full
tests/testsuite passes apart fromWebRequest::Sync::PostAfterRedirect, which fails identically on stock here; it needs a live endpoint this machine does not have.
Tested on Linux/aarch64 against master. Not tested on x86_64 locally.
Marking ready for review.
—
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 approved this pull request.
Thanks, I do see how this fixes a problem, even though I also think it could probably be worked around in the application relatively simply by just using CallAfter() to delay unregistering until the next iteration.
I'm a bit surprised by the need to use a map here, it looks like we could use a wrapping pointer instead (i.e. a struct containing both the pointer to an actual handler that would be reset to null when it's unregistered). But I'm not sure hash map lookup overhead is really a problem, so let's merge this to fix the bug instead of trying to find the most optimal solution.
But if you're interested in backporting this to 3.2, I think it would make sense to switch to a non-map-based solution as it looks like it might be even simpler — and should be simpler to backport too.
—
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#26924)Thanks for the review.
On the CallAfter() workaround — it works when the application owns the source, but not for the case this came from. There the source belongs to wxWebSessionCURL, and it is destroyed from inside wx's own curl socket callback during Dispatch():
wxEpollDispatcher::UnregisterFD
libwx_baseu [~wxEventLoopSource]
libwx_baseu_net [wxWebSessionCURL removes the source]
libwx_baseu_net [wx's curl socket callback]
libcurl x7 [curl_multi_socket_action, transfer completion]
libwx_baseu [... wxEpollDispatcher::Dispatch ...]
wxEventLoopManual::ProcessEvents()
wxAppConsoleBase::MainLoop()
amuled(CamuleDaemonApp::OnRun())
There is no application frame between OnRun() and the wx frames — the application never sees these sources, so it has nothing to defer. Any wxWebRequest user on the curl backend is exposed with no workaround available, which is also why a 3.2 backport matters: that is the series distributions ship.
So yes, I would like the backport, and I will redo this with the wrapper-pointer approach. One design point I would rather get right up front: the wrapper cannot be freed on unregister, since the in-flight epoll_event batch still points at it. I plan to keep one wrapper per descriptor, owned by the dispatcher and reused, with the handler field nulled rather than freed. Say if you had something different in mind.
Would you prefer I push that here, or merge this as-is and take the wrapper version as a follow-up?
—
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#26924)On the
CallAfter()workaround — it works when the application owns the source, but not for the case this came from. There the source belongs towxWebSessionCURL, and it is destroyed from inside wx's own curl socket callback duringDispatch()
I see, thanks. I guess we could add CallAfter() there in 3.2, but fixing it at wxEpollDispatcher level is preferable, of course, as long as wx needs to be updated anyhow.
So yes, I would like the backport, and I will redo this with the wrapper-pointer approach. One design point I would rather get right up front: the wrapper cannot be freed on unregister, since the in-flight
epoll_eventbatch still points at it. I plan to keep one wrapper per descriptor, owned by the dispatcher and reused, with the handler field nulled rather than freed. Say if you had something different in mind.
No, I thought of something like that. I didn't spend much time on it (I'm trying to review as many PRs as possible and there are too many of them nowadays to be able to spend significant time on any of them...), but it looks like it should work — sorry in advance if I overlooked something.
Would you prefer I push that here, or merge this as-is and take the wrapper version as a follow-up?
I'll merge this as is because I made some (very minor) changes to the test, so please open a new PR. TIA!
—
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#26924)One data point before I start on the wrapper: I tried backporting this as-is to 3.2 to see how awkward it would be. It needed only cosmetic changes — nullptr → NULL, override → wxOVERRIDE — builds clean there, and the reproducer from the description passes 100/100 (wx 3.2.12, Linux/aarch64, ASan build).
So backportability by itself isn't an argument against the map. Do you still prefer the wrapper on design grounds — in which case I'll write it against master and then backport that — or would you take the map version for 3.2 as-is?
Either works for me; I just didn't want to redo it if the map version is acceptable.
—
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#26924)Map version is acceptable, as I said I'm not sure if it can really become a problem to use it, but I think the wrapper version would be better. I obviously haven't done any benchmarks or anything but the wrapper version really shouldn't have any problems, even in theory.
—
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#26924)Wrapper version is up as #26930.
epoll_event::data.ptr now points at a dispatcher-owned entry holding the handler, one per descriptor and reused, with the handler cleared rather than the entry freed on unregister. Dispatch() does not touch any container, so the lock guarding it is off the dispatching path entirely.
Verified on master (Linux/aarch64, ASan, forced rebuild each side): reproducer 200/200 clean against 200/200 heap-use-after-free on stock, tests/events/evtsource.cpp passes against fails, and the full suite is 487/488 with the one failure, WebRequest::Sync::PostAfterRedirect, failing identically on unpatched master here.
Whichever of the two you prefer to keep, close the other one at your convenience. I will follow up with the 3.2 backport of whichever you settle on.
—
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.