Fix MSW wxFileSystemWatcher removal races (PR #26866)
Richard Thomson
Cancel outstanding overlapped directory reads before destroying their buffers, and protect the IOCP watch lists shared between the main thread and worker thread. Also handle cancelled completions as normal watch removal so removed watches are not processed or reissued.
Fixes #22381
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26866
Commit Summary
- 870cd28 Fix MSW wxFileSystemWatcher removal races
File Changes
(2 files)
- M include/wx/msw/private/fswatcher.h (99)
- M src/msw/fswatcher.cpp (19)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26866.patch
- https://github.com/wxWidgets/wxWidgets/pull/26866.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.
VZ
@vadz commented on this pull request.
Thanks, there are certainly problems in the existing code, but I'm not sure if this really solves all of them if we still need the lock in the dtor, could you please check if this is really needed?
TIA!
In include/wx/msw/private/fswatcher.h:
> @@ -34,27 +35,77 @@ class wxFSWatchEntryMSW : public wxFSWatchInfo
// get handle for this path
m_handle = OpenDir(m_path);
m_overlapped = (OVERLAPPED*)calloc(1, sizeof(OVERLAPPED));
+ if ( m_overlapped )
Not sure if it's worth it, calloc() is never going to fail in practice as we will have crashed long before there is not enough memory for an OVERLAPPED struct remaining.
In include/wx/msw/private/fswatcher.h:
> @@ -34,27 +35,77 @@ class wxFSWatchEntryMSW : public wxFSWatchInfo
// get handle for this path
m_handle = OpenDir(m_path);
m_overlapped = (OVERLAPPED*)calloc(1, sizeof(OVERLAPPED));
+ if ( m_overlapped )
+ {
+ m_overlapped->hEvent = ::CreateEvent(nullptr, TRUE, FALSE, nullptr);
It would be nice to have some comment somewhere explaining how this event is used.
In include/wx/msw/private/fswatcher.h:
> + wxLogSysError(_("Failed to create event for monitoring \"%s\"."),
+ m_path);
This is not really worth reporting to the user, especially because it's probably never going to happen anyhow (see comment about calloc() above) so I'd just use
- wxLogSysError(_("Failed to create event for monitoring \"%s\"."),
- m_path);
+ wxLogLastError("Failed to create event for path monitoring");
In include/wx/msw/private/fswatcher.h:
> wxZeroMemory(m_buffer);
}
virtual ~wxFSWatchEntryMSW()
{
wxLogTrace(wxTRACE_FSWATCHER, "Deleting entry '%s'", m_path);
- if (m_handle != INVALID_HANDLE_VALUE)
+ if ( m_handle != INVALID_HANDLE_VALUE )
+ {
+ if ( m_overlapped )
+ {
+ // The event buffer and OVERLAPPED object must remain alive
+ // until the asynchronous read has really completed.
+ if ( !CancelIoEx(m_handle, m_overlapped) )
Very minor, but we usually use this convention (as for GetLastError() below):
- if ( !CancelIoEx(m_handle, m_overlapped) ) + if ( !::CancelIoEx(m_handle, m_overlapped) )
In include/wx/msw/private/fswatcher.h:
> wxZeroMemory(m_buffer);
}
virtual ~wxFSWatchEntryMSW()
{
wxLogTrace(wxTRACE_FSWATCHER, "Deleting entry '%s'", m_path);
- if (m_handle != INVALID_HANDLE_VALUE)
+ if ( m_handle != INVALID_HANDLE_VALUE )
+ {
+ if ( m_overlapped )
+ {
+ // The event buffer and OVERLAPPED object must remain alive
+ // until the asynchronous read has really completed.
+ if ( !CancelIoEx(m_handle, m_overlapped) )
+ {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_NOT_FOUND )
+ {
+ wxLogSysError(err,
Again, probably not really worth reporting to the user, not to ask translators to translate this, so should be a debug log instead.
In include/wx/msw/private/fswatcher.h:
> wxZeroMemory(m_buffer);
}
virtual ~wxFSWatchEntryMSW()
{
wxLogTrace(wxTRACE_FSWATCHER, "Deleting entry '%s'", m_path);
- if (m_handle != INVALID_HANDLE_VALUE)
+ if ( m_handle != INVALID_HANDLE_VALUE )
+ {
+ if ( m_overlapped )
+ {
+ // The event buffer and OVERLAPPED object must remain alive
+ // until the asynchronous read has really completed.
+ if ( !CancelIoEx(m_handle, m_overlapped) )
+ {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_NOT_FOUND )
Why is it fine to get NOT_FOUND?
In include/wx/msw/private/fswatcher.h:
> + if ( !GetOverlappedResult(m_handle, m_overlapped, &bytes, + TRUE) )⬇️ Suggested change
- if ( !GetOverlappedResult(m_handle, m_overlapped, &bytes, - TRUE) ) + if ( !::GetOverlappedResult(m_handle, m_overlapped, &bytes, + TRUE /* wait */) )
In include/wx/msw/private/fswatcher.h:
> + {
+ wxLogSysError(err,
+ _("Unable to cancel the watch for '%s'"),
+ m_path);
+ }
+ }
+ else
+ {
+ DWORD bytes = 0;
+ if ( !GetOverlappedResult(m_handle, m_overlapped, &bytes,
+ TRUE) )
+ {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_OPERATION_ABORTED )
+ {
+ wxLogSysError(err,
Same as above.
In include/wx/msw/private/fswatcher.h:
> + {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_OPERATION_ABORTED )
+ {
+ wxLogSysError(err,
+ _("Unable to finish cancelling the "
+ "watch for '%s'"),
+ m_path);
+ }
+ }
+ }
+ }
+
+ if ( !CloseHandle(m_handle) )
+ {
+ wxLogSysError(_("Unable to close the handle for '%s'"), m_path);
I realize that this is the existing code, but the same logic applies to it and we shouldn't report this to the user either.
In include/wx/msw/private/fswatcher.h:
> {
- wxLogSysError(_("Unable to close the handle for '%s'"),
- m_path);
+ wxLogSysError(_("Unable to close the watch event for '%s'"),
Same as for the other logs here.
In include/wx/msw/private/fswatcher.h:
> {
- if (!CloseHandle(m_iocp))
+ wxCriticalSectionLocker lock(m_critsect);
This doesn't make sense to me... If somebody else can lock the CS belonging to the object being deleted, it means there is a race when deleting this object. This race must be fixed and there should be no other threads using it by the time it's destroyed, otherwise we'd still have a crash on our hands!
In include/wx/msw/private/fswatcher.h:
> // associate with IOCP
HANDLE ret = CreateIoCompletionPort(watch->GetHandle(), m_iocp,
(ULONG_PTR)watch.get(), 0);
- if (ret == nullptr)
+ if ( ret == nullptr )
Please avoid mixing up whitespace-only changes with real ones.
In include/wx/msw/private/fswatcher.h:
> + if ( err == ERROR_ACCESS_DENIED && *watch && + !wxFileName::DirExists((*watch)->GetPath()) )
The diff is smaller than it looks:
⬇️ Suggested change- if ( err == ERROR_ACCESS_DENIED && *watch && - !wxFileName::DirExists((*watch)->GetPath()) ) + if ( err == ERROR_ACCESS_DENIED && + *watch && !wxFileName::DirExists((*watch)->GetPath()) )
—
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.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26866)[...] I'm not sure if this really solves all of them if we still need the lock in the dtor, could you please check if this is really needed?
You're right, the destructor lock doesn't buy us anything here. By the time wxIOCPService is destroyed, wxFSWatcherImplMSW::~wxFSWatcherImplMSW() has already told the worker thread to finish and waited for it, so there shouldn't be any concurrent access to these watch lists any more.
If another thread could still lock or otherwise use this wxIOCPService while its destructor is running, that would be an object lifetime bug rather than something this critical section could make safe.
I've removed the lock and kept the container cleanup.
—
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.
Richard Thomson
@LegalizeAdulthood commented on this pull request.
In include/wx/msw/private/fswatcher.h:
> @@ -34,27 +35,77 @@ class wxFSWatchEntryMSW : public wxFSWatchInfo
// get handle for this path
m_handle = OpenDir(m_path);
m_overlapped = (OVERLAPPED*)calloc(1, sizeof(OVERLAPPED));
+ if ( m_overlapped )
Agreed, this check isn't useful in practice and just adds old C-style defensive noise here. If allocating an OVERLAPPED ever fails, we'd already be in a much broader out-of-memory failure mode.
I've removed the calloc() null check and left only the CreateEvent() failure handling, which is the meaningful initialization check for whether the watch can actually be used.
—
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.
Richard Thomson
> @@ -34,27 +35,77 @@ class wxFSWatchEntryMSW : public wxFSWatchInfo
// get handle for this path
m_handle = OpenDir(m_path);
m_overlapped = (OVERLAPPED*)calloc(1, sizeof(OVERLAPPED));
+ if ( m_overlapped )
+ {
+ m_overlapped->hEvent = ::CreateEvent(nullptr, TRUE, FALSE, nullptr);
Added a comment explaining this now.
The event isn't used for normal notification delivery -- those completions still come through the IOCP. It's there so that, after CancelIoEx(), GetOverlappedResult(..., TRUE) has a waitable handle and we don't free the OVERLAPPED object or its buffer before the pending ReadDirectoryChangesW() call has really finished.
—
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.
Richard Thomson
> + wxLogSysError(_("Failed to create event for monitoring \"%s\"."),
+ m_path);
done
—
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.
Richard Thomson
> + {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_OPERATION_ABORTED )
+ {
+ wxLogSysError(err,
+ _("Unable to finish cancelling the "
+ "watch for '%s'"),
+ m_path);
+ }
+ }
+ }
+ }
+
+ if ( !CloseHandle(m_handle) )
+ {
+ wxLogSysError(_("Unable to close the handle for '%s'"), m_path);
done
—
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.
Richard Thomson
> wxZeroMemory(m_buffer);
}
virtual ~wxFSWatchEntryMSW()
{
wxLogTrace(wxTRACE_FSWATCHER, "Deleting entry '%s'", m_path);
- if (m_handle != INVALID_HANDLE_VALUE)
+ if ( m_handle != INVALID_HANDLE_VALUE )
+ {
+ if ( m_overlapped )
+ {
+ // The event buffer and OVERLAPPED object must remain alive
+ // until the asynchronous read has really completed.
+ if ( !CancelIoEx(m_handle, m_overlapped) )
+ {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_NOT_FOUND )
+ {
+ wxLogSysError(err,
done
—
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.
Richard Thomson
> {
- wxLogSysError(_("Unable to close the handle for '%s'"),
- m_path);
+ wxLogSysError(_("Unable to close the watch event for '%s'"),
done
—
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.
Richard Thomson
> wxZeroMemory(m_buffer);
}
virtual ~wxFSWatchEntryMSW()
{
wxLogTrace(wxTRACE_FSWATCHER, "Deleting entry '%s'", m_path);
- if (m_handle != INVALID_HANDLE_VALUE)
+ if ( m_handle != INVALID_HANDLE_VALUE )
+ {
+ if ( m_overlapped )
+ {
+ // The event buffer and OVERLAPPED object must remain alive
+ // until the asynchronous read has really completed.
+ if ( !CancelIoEx(m_handle, m_overlapped) )
done, for this and all other Win32 API calls.
—
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.
Richard Thomson
> // associate with IOCP
HANDLE ret = CreateIoCompletionPort(watch->GetHandle(), m_iocp,
(ULONG_PTR)watch.get(), 0);
- if (ret == nullptr)
+ if ( ret == nullptr )
reverted
—
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.
Richard Thomson
> {
- if (!CloseHandle(m_iocp))
+ wxCriticalSectionLocker lock(m_critsect);
Agreed, this was wrong in the destructor.
The lock is useful while the service is alive, because Add(), ScheduleForRemoval(), and CompleteRemoval() can touch the watch lists from different threads. But by the time wxIOCPService is being destroyed, wxFSWatcherImplMSW::~wxFSWatcherImplMSW() should already have stopped and joined the IOCP worker thread.
So if another thread could still take this critical section during ~wxIOCPService(), that would already be an object lifetime race, and the destructor lock wouldn't make it safe.
I've removed the destructor lock, while keeping the locks in the live-object methods that protect the shared watch lists.
—
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.
Richard Thomson
> + if ( err == ERROR_ACCESS_DENIED && *watch && + !wxFileName::DirExists((*watch)->GetPath()) )
reverted
—
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.
Richard Thomson
@LegalizeAdulthood pushed 1 commit.
- 9656276 Fix MSW wxFileSystemWatcher removal races
—
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.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26866)I think I've addressed everything.
—
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 commented on this pull request.
In include/wx/msw/private/fswatcher.h:
> wxZeroMemory(m_buffer);
}
virtual ~wxFSWatchEntryMSW()
{
wxLogTrace(wxTRACE_FSWATCHER, "Deleting entry '%s'", m_path);
- if (m_handle != INVALID_HANDLE_VALUE)
+ if ( m_handle != INVALID_HANDLE_VALUE )
+ {
+ if ( m_overlapped )
+ {
+ // The event buffer and OVERLAPPED object must remain alive
+ // until the asynchronous read has really completed.
+ if ( !CancelIoEx(m_handle, m_overlapped) )
+ {
+ const DWORD err = ::GetLastError();
+ if ( err != ERROR_NOT_FOUND )
I'm still not sure about this one and think it might merit a comment. I guess it's possible that it has already completed and NOT_FOUND is returned in this case, but I'm really not sure, could you please explain?
—
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
> + if ( !GetOverlappedResult(m_handle, m_overlapped, &bytes, + TRUE) )
I'd still apply this.
—
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#26866)Thanks for the updates, but I still have one question remaining (I think you might not have noticed it because GitHub web UI collapsed the comments in the middle, you need to expand them to see all of them).
—
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#26866)@LegalizeAdulthood I'd really like to merge this one but there is still one question remaining, could you please check it? 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.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26866)It might take me a few weeks to respond to questions on these open PRs because I'm preparing for Vintage Computing Festival Midwest. So if I don't respond for a while, don't take that as me ignoring questions and not getting back to the PRs :).
The review process for these PRs has been very helpful and has improved many of the PRs, so I take it seriously. I also have ~100 bug fixes in my personal queue, so there's more to come :).
—
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.