Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VRC7 data race and use-after-free when reloading/closing modules #106

Conversation

nyanpasu64
Copy link
Collaborator

@nyanpasu64 nyanpasu64 commented Nov 23, 2021

FamiTracker doesn't consistently acquire mutexes when the GUI and audio threads operate on CSoundGen. This results in data races, and in the case of the VRC7, the GUI or audio thread can call CAPU::ChangeMachineRate > CVRC7::SetSampleSpeed > OPLL_delete, before the other thread calls OPLL_delete or OPLL_reset.

The use-after-free results in a crash on ASAN builds (link), and can be reliably reproduced by adding a 1-second sleep in CVRC7::SetSampleSpeed(), between calling OPLL_delete() and OPLL_new(). See https://github.com/nyanpasu64/Dn-FamiTracker/tree/asan-fix-vrc7 for a reproducer.

Fix

I added extra locks to CSoundGen::m_csAPULock when necessary. I'm not sure that the current set of locks is sufficient, but ASAN is no longer reporting errors when I open or switch documents, change module properties, or press F12.

Additionally I switched CSoundGen::m_csAPULock from CCriticalSection and CSingleLock (which didn't work properly), to std::mutex and std::unique_lock. I'm not sure why CSingleLock didn't work. Maybe it's because Lock() moves upon return (in the absence of NRVO), and the moved-from object's destructor unlocks the CCriticalSection. Maybe the lock is released upon std::this_thread::sleep_for(). It's not because I tried to lock the same mutex twice in the same thread (since MSVC's std::mutex throws a std::system_error in that case). In any case, std::unique_lock works properly.

Discussion

Theoretically it's safer to null out m_pOPLLInt before calling OPLL_new (it would be more exception-safe, except not because OPLL_new doesn't throw exceptions), but doing so would prevent ASAN from catching the data races (Windows MSVC doesn't have TSAN right now).

I'm not actually sure if the std::atomic is necessary. Not changing it is technically a data race, but not a big deal. It might be faster to use relaxed access, and switch the compound assignments to something that doesn't write then read(? unsure).

There is no wasted write-then-read. See https://en.cppreference.com/w/cpp/atomic/atomic/operator%3D:

Unlike most assignment operators, the assignment operators for atomic types do not return a reference to their left-hand arguments. They return a copy of the stored value instead.

FamiTracker doesn't consistently acquire mutexes when the GUI and audio
threads operate on CSoundGen. This results in data races, and in the
case of the VRC7, the GUI or audio thread can call
CAPU::ChangeMachineRate > CVRC7::SetSampleSpeed > OPLL_delete, before
the other thread calls OPLL_delete or OPLL_reset.

The use-after-free results in a crash on ASAN builds, and can be
reliably reproduced by adding a 1-second sleep in
CVRC7::SetSampleSpeed(), between calling OPLL_delete() and OPLL_new().
See https://github.com/nyanpasu64/Dn-FamiTracker/tree/asan-fix-vrc7 for
a reproducer.

## Fix

I added extra locks to CSoundGen::m_csAPULock when necessary. I'm not
sure that the current set of locks is sufficient, but ASAN is no longer
reporting errors when I open or switch documents, change module
properties, or press F12.

Additionally I switched CSoundGen::m_csAPULock from CCriticalSection and
CSingleLock (which didn't work properly), to std::mutex and
std::unique_lock. I'm not sure why CSingleLock didn't work. Maybe it's
because Lock() moves upon return (in the absence of NRVO), and the
moved-from object's destructor unlocks the CCriticalSection. Maybe the
lock is released upon std::this_thread::sleep_for(). It's not because I
tried to lock the same mutex twice in the same thread (since MSVC's
std::mutex throws a std::system_error in that case). In any case,
std::unique_lock works properly.

## Discussion

Theoretically it's safer to null out m_pOPLLInt before calling OPLL_new
(it would be more exception-safe, except not because OPLL_new doesn't
throw exceptions), but doing so would prevent ASAN from catching the
data races (Windows MSVC doesn't have TSAN right now).
@Gumball2415 Gumball2415 merged commit 09725f6 into Dn-Programming-Core-Management:master Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants