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 #8335: Race condition in music mixer #9450

Merged
merged 1 commit into from Jul 21, 2021

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Jul 19, 2021

Motivation / Problem

#8335

Description

Basic. Add a mutex around all _music_stream accesses.
Also removes some '\r' from the file. (Commit checker should probably check for this?)

Limitations

Might be too simplistic of a fix, IDK. There's a remaining race condition in mixer.cpp due to _setting_client accesses.

WARNING: ThreadSanitizer: data race (pid=116977)
  Write of size 1 at 0x556b0ff4cfae by thread T4 (mutexes: write M1050):
    #0 WriteValue(void*, unsigned int, long long) /home/lordaro/dev/OTTD/openttd/src/saveload/saveload.cpp:833 (openttd+0xafcb01)
    #1 IntSettingDesc::Write(void const*, int) const /home/lordaro/dev/OTTD/openttd/src/settings.cpp:505 (openttd+0xfbad60)
    #2 IntSettingDesc::MakeValueValidAndWrite(void const*, int) const /home/lordaro/dev/OTTD/openttd/src/settings.cpp:437 (openttd+0xfba932)
    #3 IntSettingDesc::ParseValue(IniItem const*, void*) const /home/lordaro/dev/OTTD/openttd/src/settings.cpp:606 (openttd+0xfbb5ca)
    #4 IniLoadSettings /home/lordaro/dev/OTTD/openttd/src/settings.cpp:599 (openttd+0xfbb433)
    #5 HandleSettingDescs /home/lordaro/dev/OTTD/openttd/src/settings.cpp:1153 (openttd+0xfbeb97)
    #6 LoadFromConfig(bool) /home/lordaro/dev/OTTD/openttd/src/settings.cpp:1216 (openttd+0xfbf073)
    #7 AfterNewGRFScan::OnNewGRFsScanned() /home/lordaro/dev/OTTD/openttd/src/openttd.cpp:434 (openttd+0xf2661e)
    #8 DoScanNewGRFFiles(NewGRFScanCallback*) /home/lordaro/dev/OTTD/openttd/src/newgrf_config.cpp:707 (openttd+0xe87859)
    #9 ScanNewGRFFiles(NewGRFScanCallback*) /home/lordaro/dev/OTTD/openttd/src/newgrf_config.cpp:725 (openttd+0xe87914)
    #10 GameLoop() /home/lordaro/dev/OTTD/openttd/src/openttd.cpp:1426 (openttd+0xf23db1)
    #11 VideoDriver::GameLoop() /home/lordaro/dev/OTTD/openttd/src/video/video_driver.cpp:37 (openttd+0xbbbef6)
    #12 VideoDriver::GameThread() /home/lordaro/dev/OTTD/openttd/src/video/video_driver.cpp:44 (openttd+0xbbbf7f)
    #13 VideoDriver::GameThreadThunk(VideoDriver*) /home/lordaro/dev/OTTD/openttd/src/video/video_driver.cpp:81 (openttd+0xbbc12e)
    #14 StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}::operator()(char const*, void (*&&)(VideoDriver*), VideoDriver*&&) const /home/lordaro/dev/OTTD/openttd/src/video/../thread.h:65 (openttd+0xbbd8e5)
    #15 void std::__invoke_impl<void, StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*>(std::__invoke_other, StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&&&)(VideoDriver*), VideoDriver*&&)#1}, char const*&&, void (*&&)(VideoDriver*), VideoDriver*&&) /usr/include/c++/11.1.0/bits/invoke.h:61 (openttd+0xbbeacc)
    #16 _ZSt8__invokeIZ14StartNewThreadIPFvP11VideoDriverEJS2_EEbPSt6threadPKcOT_DpOT0_EUlS8_OS4_OS2_E_JS8_S4_S2_EENSt15__invoke_resultIS9_JDpSB_EE4typeESA_SD_ /usr/include/c++/11.1.0/bits/invoke.h:96 (openttd+0xbbe91d)
    #17 void std::thread::_Invoker<std::tuple<StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*> >::_M_invoke<0ul, 1ul, 2ul, 3ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) /usr/include/c++/11.1.0/bits/std_thread.h:253 (openttd+0xbbe77a)
    #18 std::thread::_Invoker<std::tuple<StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*> >::operator()() /usr/include/c++/11.1.0/bits/std_thread.h:260 (openttd+0xbbe6d2)
    #19 std::thread::_State_impl<std::thread::_Invoker<std::tuple<StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*> > >::_M_run() /usr/include/c++/11.1.0/bits/std_thread.h:211 (openttd+0xbbe68c)
    #20 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:82 (libstdc++.so.6+0xd33c3)

  Previous read of size 1 at 0x556b0ff4cfae by thread T2 (mutexes: write M923):
    #0 MxMixSamples(void*, unsigned int) /home/lordaro/dev/OTTD/openttd/src/mixer.cpp:167 (openttd+0xdeff32)
    #1 fill_sound_buffer /home/lordaro/dev/OTTD/openttd/src/sound/sdl2_s.cpp:31 (openttd+0xb99abd)
    #2 <null> <null> (libSDL2-2.0.so.0+0x23a9f)

  Location is global '_settings_client' of size 648 at 0x556b0ff4cd80 (openttd+0x000003701fae)

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got no other idea how to prevent _music_stream from being made nullptr between the check and call, so I guess this is good enough for that.

@LordAro LordAro merged commit 6c33b4e into OpenTTD:master Jul 21, 2021
@LordAro LordAro deleted the music-stream-race branch July 21, 2021 09:32
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.

None yet

2 participants