From 0e62d6746a7f10dcd88d85da0ae8010c983d634e Mon Sep 17 00:00:00 2001 From: Casper Jeukendrup <48658420+cbjeukendrup@users.noreply.github.com> Date: Sun, 9 Feb 2025 02:49:11 +0100 Subject: [PATCH 1/2] Fix crash because of data race in `AsyncImpl::onCall` Resolves: https://github.com/musescore/MuseScore/issues/25884 We were calling `c.caller->disconnectAsync` after `c.f->call()`, not protected by any mutex. The problem is that `c.caller` is deleted by another thread, between the mutex-protected section and the `disconnectAsync` call. A solution is to move the `disconnectAsync` call inside the mutex-protected section. It does not really matter that it happens before `c.f->call()` this way; one could even argue it's better this way, because it belongs together with `m_calls.erase(it)`. This way, we can be certain that `c.caller` has not been deleted by another thread iff it is found in `m_calls`: when `c.caller` is being deleted (see `~Asyncable`), it calls its `disconnectAll()` method, which calls `AsyncImpl::disconnectAsync`, which uses the mutex and removes the `Asyncable` from `m_calls`. A remaining potential problem now, is that `c.caller` may be deleted during `c.f->call()`, which may be problematic inside `c.f->call()`. I'm not sure if it is the responsibility of `AsyncImpl` to mitigate that. If it is, that will either be challenging, or require the use of `recursive_mutex`. --- .../thirdparty/kors_async/async/internal/asyncimpl.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp b/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp index efeaa78f9a01a..12c78217d2514 100644 --- a/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp +++ b/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp @@ -79,14 +79,15 @@ void AsyncImpl::onCall(uint64_t key) } c = it->second; + m_calls.erase(it); + + if (c.caller) { + c.caller->disconnectAsync(this); + } } c.f->call(); - if (c.caller) { - c.caller->disconnectAsync(this); - } - delete c.f; } From 447baec84b2f95c619ad21a643365cb47e24d948 Mon Sep 17 00:00:00 2001 From: Casper Jeukendrup <48658420+cbjeukendrup@users.noreply.github.com> Date: Sun, 9 Feb 2025 02:49:27 +0100 Subject: [PATCH 2/2] Simplify `AsyncImpl::disconnectAsync` --- .../kors_async/async/internal/asyncimpl.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp b/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp index 12c78217d2514..2357bf2302199 100644 --- a/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp +++ b/src/framework/global/thirdparty/kors_async/async/internal/asyncimpl.cpp @@ -36,18 +36,13 @@ AsyncImpl* AsyncImpl::instance() void AsyncImpl::disconnectAsync(Asyncable* caller) { std::lock_guard locker(m_mutex); - uint64_t key = 0; - std::map::const_iterator it = m_calls.cbegin(), end = m_calls.cend(); - for (; it != end; ++it) { + + for (auto it = m_calls.cbegin(), end = m_calls.cend(); it != end; ++it) { if (it->second.caller == caller) { - key = it->first; + m_calls.erase(it); break; } } - - if (key) { - m_calls.erase(key); - } } void AsyncImpl::call(Asyncable* caller, IFunction* f, const std::thread::id& th)