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

Jump to bottom

Thread safety issues detected by ThreadSanitizer #8712

Closed
JGRennison opened this issue Feb 21, 2021 · 9 comments
Closed

Thread safety issues detected by ThreadSanitizer #8712

JGRennison opened this issue Feb 21, 2021 · 9 comments

Comments

@JGRennison
Copy link
Contributor

JGRennison commented Feb 21, 2021 •
edited

Version of OpenTTD

e1b1608

(These have been here for a long time and are not caused by the recent changes).

Expected result

No potentially problematic thread safety issues.

Actual result

ThreadSantizer output from testing e1b1608 on Linux using SDL2

I've excluded output related to PulseAudio internals.

==================
WARNING: ThreadSanitizer: data race (pid=1525057)
  Read of size 1 at 0x555614643421 by thread T3 (mutexes: write M660):
    #0 IsGenerateWorldThreaded() /home/jgr/openttd/trunk4/src/genworld.cpp:68 (openttd+0xe86c4a)
    #1 _SetGeneratingWorldProgress /home/jgr/openttd/trunk4/src/genworld_gui.cpp:1316 (openttd+0xe890c9)
    #2 SetGeneratingWorldProgress(GenWorldProgress, unsigned int) /home/jgr/openttd/trunk4/src/genworld_gui.cpp:1382 (openttd+0xe895ad)
    #3 _GenerateWorld /home/jgr/openttd/trunk4/src/genworld.cpp:108 (openttd+0xe86ed2)
    #4 StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}::operator()(char const*, void (*&&)()) const /home/jgr/openttd/trunk4/src/thread.h:56 (openttd+0xbfc03f)
    #5 void std::__invoke_impl<void, StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}, char const*, void (*)()>(std::__invoke_other, StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&&&)())#1}, char const*&&, void (*&&)()) /usr/include/c++/9/bits/invoke.h:60 (openttd+0xbfe5d3)
    #6 std::__invoke_result<StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}, char const*, void (*)()>::type std::__invoke<StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}, char const*, void (*)()>(void (*&&)(), char const*&&, void (*&&)()) /usr/include/c++/9/bits/invoke.h:95 (openttd+0xbfe1e5)
    #7 void std::thread::_Invoker<std::tuple<StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}, char const*, void (*)()> >::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) /usr/include/c++/9/thread:244 (openttd+0xbfdf8e)
    #8 std::thread::_Invoker<std::tuple<StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}, char const*, void (*)()> >::operator()() /usr/include/c++/9/thread:251 (openttd+0xbfdea8)
    #9 std::thread::_State_impl<std::thread::_Invoker<std::tuple<StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)())::{lambda(char const*, void (*&&)())#1}, char const*, void (*)()> > >::_M_run() /usr/include/c++/9/thread:195 (openttd+0xbfde08)
    #10 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous write of size 1 at 0x555614643421 by main thread:
    #0 WaitTillGeneratedWorld() /home/jgr/openttd/trunk4/src/genworld.cpp:253 (openttd+0xe87676)
    #1 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:842 (openttd+0x1117bc3)
    #2 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Location is global '_gw' of size 48 at 0x555614643420 (openttd+0x0000020d6421)

  Mutex M660 (0x555616f37fa0) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xae5a4f)
    #2 std::mutex::lock() /usr/include/c++/9/bits/std_mutex.h:100 (openttd+0xae7420)
    #3 std::unique_lock<std::mutex>::lock() /usr/include/c++/9/bits/unique_lock.h:141 (openttd+0xbf82c9)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:833 (openttd+0x1117b96)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Thread T3 'ottd:genworld' (tid=1525063, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 bool StartNewThread<void (*)()>(std::thread*, char const*, void (*&&)()) /home/jgr/openttd/trunk4/src/thread.h:52 (openttd+0xbfc0fe)
    #3 GenerateWorld(GenWorldMode, unsigned int, unsigned int, bool) /home/jgr/openttd/trunk4/src/genworld.cpp:333 (openttd+0xe87a64)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:841 (openttd+0x1117bbe)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

SUMMARY: ThreadSanitizer: data race /home/jgr/openttd/trunk4/src/genworld.cpp:68 in IsGenerateWorldThreaded()
==================

GenWorldInfo::quit_thread is not atomic, but is modified whilst the generate world thread is running.
This is technically racy, but not likely to lead to any observable problem.

==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=1525057)
  Cycle in lock order graph: M660 (0x555616f37fa0) => M661 (0x555616f37fe0) => M660

  Mutex M661 acquired here while holding mutex M660 in main thread:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xae5a4f)
    #2 std::mutex::lock() /usr/include/c++/9/bits/std_mutex.h:100 (openttd+0xae7420)
    #3 std::unique_lock<std::mutex>::lock() /usr/include/c++/9/bits/unique_lock.h:141 (openttd+0xbf82c9)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:834 (openttd+0x1117ba5)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

    Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message

  Mutex M660 acquired here while holding mutex M661 in main thread:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xae5a4f)
    #2 std::mutex::lock() /usr/include/c++/9/bits/std_mutex.h:100 (openttd+0xae7420)
    #3 DrawDirtyBlocks() /home/jgr/openttd/trunk4/src/gfx.cpp:1480 (openttd+0xe9d5f6)
    #4 UpdateWindows() /home/jgr/openttd/trunk4/src/window.cpp:3202 (openttd+0x1451691)
    #5 VideoDriver::Tick() /home/jgr/openttd/trunk4/src/video/video_driver.cpp:60 (openttd+0xd2c417)
    #6 VideoDriver_SDL::LoopOnce() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:789 (openttd+0xd2012e)
    #7 VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:841 (openttd+0xd204f8)
    #8 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:851 (openttd+0x1117c1e)
    #9 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) (/usr/lib/x86_64-linux-gnu/libtsan.so.0+0x5271c) in pthread_mutex_lock
==================

I looked into this some time ago and couldn't find a way in which this could actually lead to a deadlock.
I presume that that is still the case with the recent changes, but I haven't combed through them all yet.

==================
WARNING: ThreadSanitizer: data race (pid=1525057)
  Write of size 1 at 0x555616f37f81 by thread T4 (mutexes: write M661):
    #0 SetModalProgress(bool) /home/jgr/openttd/trunk4/src/progress.cpp:33 (openttd+0x1149786)
    #1 DoScanNewGRFFiles(NewGRFScanCallback*) /home/jgr/openttd/trunk4/src/newgrf_config.cpp:721 (openttd+0x105918c)
    #2 StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)::{lambda(char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)#1}::operator()(char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&) const /home/jgr/openttd/trunk4/src/thread.h:56 (openttd+0x105bc5f)
    #3 void std::__invoke_impl<void, StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)::{lambda(char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)#1}, char const*, void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::__invoke_other, StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)::{lambda(char const*, void (*&&&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)#1}, char const*&&, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&) /usr/include/c++/9/bits/invoke.h:60 (openttd+0x10691ea)
    #4 _ZSt8__invokeIZ14StartNewThreadIPFvP18NewGRFScanCallbackEJS2_EEbPSt6threadPKcOT_DpOT0_EUlS8_OS4_OS2_E_JS8_S4_S2_EENSt15__invoke_resultIS9_JDpSB_EE4typeESA_SD_ /usr/include/c++/9/bits/invoke.h:95 (openttd+0x1069013)
    #5 void std::thread::_Invoker<std::tuple<StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)::{lambda(char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)#1}, char const*, void (*)(NewGRFScanCallback*), NewGRFScanCallback*> >::_M_invoke<0ul, 1ul, 2ul, 3ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) /usr/include/c++/9/thread:244 (openttd+0x1068e50)
    #6 std::thread::_Invoker<std::tuple<StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)::{lambda(char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)#1}, char const*, void (*)(NewGRFScanCallback*), NewGRFScanCallback*> >::operator()() /usr/include/c++/9/thread:251 (openttd+0x1068d9e)
    #7 std::thread::_State_impl<std::thread::_Invoker<std::tuple<StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)::{lambda(char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&)#1}, char const*, void (*)(NewGRFScanCallback*), NewGRFScanCallback*> > >::_M_run() /usr/include/c++/9/thread:195 (openttd+0x1068d50)
    #8 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous write of size 1 at 0x555616f37f81 by main thread (mutexes: write M696):
    #0 IsFirstModalProgressLoop() /home/jgr/openttd/trunk4/src/progress.cpp:44 (openttd+0x11497d0)
    #1 DrawDirtyBlocks() /home/jgr/openttd/trunk4/src/gfx.cpp:1474 (openttd+0xe9d577)
    #2 UpdateWindows() /home/jgr/openttd/trunk4/src/window.cpp:3202 (openttd+0x1451691)
    #3 VideoDriver::Tick() /home/jgr/openttd/trunk4/src/video/video_driver.cpp:60 (openttd+0xd2c417)
    #4 VideoDriver_SDL::LoopOnce() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:789 (openttd+0xd2012e)
    #5 VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:841 (openttd+0xd204f8)
    #6 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:851 (openttd+0x1117c1e)
    #7 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Location is global '_first_in_modal_loop' of size 1 at 0x555616f37f81 (openttd+0x0000049caf81)

  Mutex M661 (0x555616f37fe0) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xae5a4f)
    #2 std::mutex::lock() /usr/include/c++/9/bits/std_mutex.h:100 (openttd+0xae7420)
    #3 std::unique_lock<std::mutex>::lock() /usr/include/c++/9/bits/unique_lock.h:141 (openttd+0xbf82c9)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:834 (openttd+0x1117ba5)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Mutex M696 (0x7b0c00039f60) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xd1d239)
    #2 __gthread_recursive_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:811 (openttd+0xd2107e)
    #3 std::recursive_mutex::lock() /usr/include/c++/9/mutex:106 (openttd+0xd211f8)
    #4 std::unique_lock<std::recursive_mutex>::lock() <null> (openttd+0xd22ebd)
    #5 std::unique_lock<std::recursive_mutex>::unique_lock(std::recursive_mutex&) /usr/include/c++/9/bits/unique_lock.h:71 (openttd+0xd21e62)
    #6 VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:813 (openttd+0xd202cd)
    #7 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:851 (openttd+0x1117c1e)
    #8 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Thread T4 'ottd:newgrf-scan' (tid=1525066, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 bool StartNewThread<void (*)(NewGRFScanCallback*), NewGRFScanCallback*>(std::thread*, char const*, void (*&&)(NewGRFScanCallback*), NewGRFScanCallback*&&) /home/jgr/openttd/trunk4/src/thread.h:52 (openttd+0x105bd34)
    #3 ScanNewGRFFiles(NewGRFScanCallback*) /home/jgr/openttd/trunk4/src/newgrf_config.cpp:736 (openttd+0x10592f6)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:849 (openttd+0x1117bf3)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

SUMMARY: ThreadSanitizer: data race /home/jgr/openttd/trunk4/src/progress.cpp:33 in SetModalProgress(bool)
==================

IsFirstModalProgressLoop() should be called before releasing the locks, instead of after it, in DrawDirtyBlocks/HasModalProgress. The resulting sleep should still be done after releasing the locks.
(I have a commit for this, but it's heavily out of date with respect to recent changes, I can make a fresh one if you want a PR).

==================
WARNING: ThreadSanitizer: data race (pid=1525057)
  Write of size 4 at 0x555614645b84 by thread T5 (mutexes: write M696):
    #0 VideoDriver_SDL::Paint() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:151 (openttd+0xd1dacb)
    #1 VideoDriver_SDL::PaintThread() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:175 (openttd+0xd1dd5b)
    #2 VideoDriver_SDL::PaintThreadThunk(VideoDriver_SDL*) /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:182 (openttd+0xd1de10)
    #3 StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)::{lambda(char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)#1}::operator()(char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&) const <null> (openttd+0xd22ac5)
    #4 void std::__invoke_impl<void, StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)::{lambda(char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)#1}, char const*, void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::__invoke_other, StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)::{lambda(char const*, void (*&&&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)#1}, char const*&&, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&) /usr/include/c++/9/bits/invoke.h:60 (openttd+0xd29d1a)
    #5 _ZSt8__invokeIZ14StartNewThreadIPFvP15VideoDriver_SDLEJS2_EEbPSt6threadPKcOT_DpOT0_EUlS8_OS4_OS2_E_JS8_S4_S2_EENSt15__invoke_resultIS9_JDpSB_EE4typeESA_SD_ /usr/include/c++/9/bits/invoke.h:95 (openttd+0xd29b43)
    #6 void std::thread::_Invoker<std::tuple<StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)::{lambda(char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)#1}, char const*, void (*)(VideoDriver_SDL*), VideoDriver_SDL*> >::_M_invoke<0ul, 1ul, 2ul, 3ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) /usr/include/c++/9/thread:244 (openttd+0xd29980)
    #7 std::thread::_Invoker<std::tuple<StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)::{lambda(char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)#1}, char const*, void (*)(VideoDriver_SDL*), VideoDriver_SDL*> >::operator()() /usr/include/c++/9/thread:251 (openttd+0xd298a8)
    #8 std::thread::_State_impl<std::thread::_Invoker<std::tuple<StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)::{lambda(char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&)#1}, char const*, void (*)(VideoDriver_SDL*), VideoDriver_SDL*> > >::_M_run() /usr/include/c++/9/thread:195 (openttd+0xd297e2)
    #9 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous read of size 4 at 0x555614645b84 by main thread (mutexes: write M661, write M660):
    #0 DoPaletteAnimations() /home/jgr/openttd/trunk4/src/gfx.cpp:1248 (openttd+0xe9c505)
    #1 GameLoop() /home/jgr/openttd/trunk4/src/openttd.cpp:1505 (openttd+0x111b53a)
    #2 VideoDriver::Tick() /home/jgr/openttd/trunk4/src/video/video_driver.cpp:41 (openttd+0xd2c21c)
    #3 VideoDriver_SDL::LoopOnce() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:789 (openttd+0xd2012e)
    #4 VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:841 (openttd+0xd204f8)
    #5 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:851 (openttd+0x1117c1e)
    #6 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Location is global '_cur_palette' of size 1032 at 0x555614645780 (openttd+0x0000020d8b84)

  Mutex M696 (0x7b0c00039f60) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xd1d239)
    #2 __gthread_recursive_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:811 (openttd+0xd2107e)
    #3 std::recursive_mutex::lock() /usr/include/c++/9/mutex:106 (openttd+0xd211f8)
    #4 std::unique_lock<std::recursive_mutex>::lock() <null> (openttd+0xd22ebd)
    #5 std::unique_lock<std::recursive_mutex>::unique_lock(std::recursive_mutex&) /usr/include/c++/9/bits/unique_lock.h:71 (openttd+0xd21e62)
    #6 VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:813 (openttd+0xd202cd)
    #7 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:851 (openttd+0x1117c1e)
    #8 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Mutex M661 (0x555616f37fe0) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xae5a4f)
    #2 std::mutex::lock() /usr/include/c++/9/bits/std_mutex.h:100 (openttd+0xae7420)
    #3 std::unique_lock<std::mutex>::lock() /usr/include/c++/9/bits/unique_lock.h:141 (openttd+0xbf82c9)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:834 (openttd+0x1117ba5)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Mutex M660 (0x555616f37fa0) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.0+0x5271c)
    #1 __gthread_mutex_lock /usr/include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749 (openttd+0xae5a4f)
    #2 std::mutex::lock() /usr/include/c++/9/bits/std_mutex.h:100 (openttd+0xae7420)
    #3 std::unique_lock<std::mutex>::lock() /usr/include/c++/9/bits/unique_lock.h:141 (openttd+0xbf82c9)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:833 (openttd+0x1117b96)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

  Thread T5 'ottd:draw-sdl' (tid=1525067, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x5ea99)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 bool StartNewThread<void (*)(VideoDriver_SDL*), VideoDriver_SDL*>(std::thread*, char const*, void (*&&)(VideoDriver_SDL*), VideoDriver_SDL*&&) <null> (openttd+0xd22b9a)
    #3 VideoDriver_SDL::MainLoop() /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:817 (openttd+0xd20378)
    #4 openttd_main(int, char**) /home/jgr/openttd/trunk4/src/openttd.cpp:851 (openttd+0x1117c1e)
    #5 main /home/jgr/openttd/trunk4/src/os/unix/unix.cpp:265 (openttd+0xbff6aa)

SUMMARY: ThreadSanitizer: data race /home/jgr/openttd/trunk4/src/video/sdl2_v.cpp:151 in VideoDriver_SDL::Paint()
==================

_cur_palette is used by both the drawing thread and the main thread so shouldn't really be modified by the main thread from within GameLoop where the locks are released.
I changed this in my branch along the lines of: JGRennison/OpenTTD-patches@4c59dfb . (This becomes a much smaller diff now that the video drivers are nicely de-duplicated :) ). I can submit a forward-ported PR of that later if this would be an acceptable mitigation.

Steps to reproduce

Build with ThreadSanitizer enabled
JGRennison added a commit to JGRennison/Upstream-OpenTTD that referenced this issue Mar 8, 2021
@JGRennison
Fix: Data race on _first_in_modal_loop in use of IsFirstModalProgress…
8ba1e1a 
…Loop

See: OpenTTD#8712
@JGRennison JGRennison mentioned this issue Mar 8, 2021
Closed
@TrueBrain TrueBrain mentioned this issue Mar 9, 2021
Merged
@TrueBrain
Copy link
Member

TrueBrain commented Mar 9, 2021 •
edited

#8830 solves 3 out of the 4 mentioned here, but I mostly noticed we have a new one with latest master (fix in #8831):

WARNING: ThreadSanitizer: data race (pid=27299)
  Read of size 4 at 0x00000255fd8c by thread T3 (mutexes: write M683):
    #0 Randomizer::Next() OpenTTD/src/core/random_func.cpp:34:19 (openttd+0xce2839)
    #1 InteractiveRandom() OpenTTD/src/core/random_func.hpp:89:29 (openttd+0x138064a)
    #2 GameLoop() OpenTTD/src/openttd.cpp:1484:2 (openttd+0x138549c)
    #3 VideoDriver::GameLoop() OpenTTD/src/video/video_driver.cpp:36:3 (openttd+0xfeb880)
    #4 VideoDriver::GameThread() OpenTTD/src/video/video_driver.cpp:43:9 (openttd+0xfeb955)
    #5 VideoDriver::GameThreadThunk(VideoDriver*) OpenTTD/src/video/video_driver.cpp:75:7 (openttd+0xfebb18)
    #6 bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::operator()(char const*, void (*&&)(VideoDriver*), VideoDriver*&&) const OpenTTD/src/video/../thread.h:52:6 (openttd+0xfeeddc)
    #7 void (*std::__invoke_impl<void, bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&), char const*, void (*)(VideoDriver*), VideoDriver*>(std::__invoke_other, VideoDriver*&&, bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)&&...))(VideoDriver*) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/invoke.h:60:14 (openttd+0xfeed1d)
    #8 std::__invoke_result<void (*)(VideoDriver*), VideoDriver*>::type std::__invoke<bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&), char const*, void (*)(VideoDriver*), VideoDriver*>(void (*&&)(VideoDriver*), VideoDriver*&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/invoke.h:95:14 (openttd+0xfeead7)
    #9 void std::thread::_Invoker<std::tuple<bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&), char const*, void (*)(VideoDriver*), VideoDriver*> >::_M_invoke<0ul, 1ul, 2ul, 3ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:244:13 (openttd+0xfeea2a)
    #10 std::thread::_Invoker<std::tuple<bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&), char const*, void (*)(VideoDriver*), VideoDriver*> >::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:251:11 (openttd+0xfee978)
    #11 std::thread::_State_impl<std::thread::_Invoker<std::tuple<bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::'lambda'(char const*, void (*&&)(VideoDriver*), VideoDriver*&&), char const*, void (*)(VideoDriver*), VideoDriver*> > >::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:195:13 (openttd+0xfee1df)
    #12 <null> <null> (libstdc++.so.6+0xd6d83)

  Previous write of size 4 at 0x00000255fd8c by main thread:
    #0 Randomizer::Next() OpenTTD/src/core/random_func.cpp:37:24 (openttd+0xce28ab)
    #1 InteractiveRandom() OpenTTD/src/video/../core/random_func.hpp:89:29 (openttd+0xfec7ea)
    #2 VideoDriver::Tick() OpenTTD/src/video/video_driver.cpp:136:3 (openttd+0xfec41a)
    #3 VideoDriver_SDL_Base::LoopOnce() OpenTTD/src/video/sdl2_v.cpp:642:8 (openttd+0xfe5f22)
    #4 VideoDriver_SDL_Base::MainLoop() OpenTTD/src/video/sdl2_v.cpp:660:3 (openttd+0xfe5fc7)
    #5 openttd_main(int, char**) OpenTTD/src/openttd.cpp:837:30 (openttd+0x13802b7)
    #6 main OpenTTD/src/os/unix/unix.cpp:265:12 (openttd+0xe65e52)

  Location is global '_interactive_random' of size 8 at 0x00000255fd88 (openttd+0x00000255fd8c)

  Mutex M683 (0x7b6400005a38) created at:
    #0 pthread_mutex_lock <null> (openttd+0xa19316)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/x86_64-linux-gnu/c++/9/bits/gthr-default.h:749:12 (openttd+0xd73d66)
    #2 std::mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_mutex.h:100:17 (openttd+0xd7c518)
    #3 std::lock_guard<std::mutex>::lock_guard(std::mutex&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_mutex.h:159:19 (openttd+0xd7ba72)
    #4 VideoDriver::Tick() OpenTTD/src/video/video_driver.cpp:152:32 (openttd+0xfec543)
    #5 VideoDriver_SDL_Base::LoopOnce() OpenTTD/src/video/sdl2_v.cpp:642:8 (openttd+0xfe5f22)
    #6 VideoDriver_SDL_Base::MainLoop() OpenTTD/src/video/sdl2_v.cpp:660:3 (openttd+0xfe5fc7)
    #7 openttd_main(int, char**) OpenTTD/src/openttd.cpp:837:30 (openttd+0x13802b7)
    #8 main OpenTTD/src/os/unix/unix.cpp:265:12 (openttd+0xe65e52)

  Thread T3 'ottd:game' (tid=27307, running) created by main thread at:
    #0 pthread_create <null> (openttd+0x9fbf4b)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xd7048)
    #2 bool StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&) OpenTTD/src/video/../thread.h:48:15 (openttd+0xfed0c4)
    #3 VideoDriver::StartGameThread() OpenTTD/src/video/video_driver.cpp:81:28 (openttd+0xfebbd9)
    #4 VideoDriver_SDL_Base::MainLoop() OpenTTD/src/video/sdl2_v.cpp:657:8 (openttd+0xfe5f8f)
    #5 openttd_main(int, char**) OpenTTD/src/openttd.cpp:837:30 (openttd+0x13802b7)
    #6 main OpenTTD/src/os/unix/unix.cpp:265:12 (openttd+0xe65e52)

SUMMARY: ThreadSanitizer: data race OpenTTD/src/core/random_func.cpp:34:19 in Randomizer::Next()

JGRennison added a commit to JGRennison/OpenTTD-patches that referenced this issue Apr 6, 2021
@JGRennison
Partially fix thread safety issues around _cur_palette
e094b7f 
Replaces: 4c59dfb
See also: OpenTTD/OpenTTD#8712
JGRennison added a commit to JGRennison/OpenTTD-patches that referenced this issue Apr 6, 2021
@JGRennison
Partially fix data race issues around _cur_palette
f2bfcc5 
See: OpenTTD/OpenTTD#8712
JGRennison added a commit to JGRennison/OpenTTD-patches that referenced this issue Apr 6, 2021
@JGRennison
Partially fix data race issues around _cur_palette
ef81729 
See: OpenTTD/OpenTTD#8712
@TrueBrain
Copy link
Member

TrueBrain commented Jun 17, 2021 •
edited

I didn't see that you were also addressing _cur_palette, but I have a fix in #9379 pending. It seems we had similar ideas in mind (especially with the parts of that code that copied to local_palette but still used _cur_palette for further access .. that was just weird code :P), but I simply moved all the code related to gfx.cpp and put a recursive_mutex around the 3 functions that are reading/writing to it. Simplified the amount of code I had to modify.

Also fixed _exit_game in #9380, leaving this list shorter and shorter. The ones I still found:

@JGRennison
Copy link
Contributor Author

I've got a commit for the modifier key race issue here: JGRennison/OpenTTD-patches@59daa57

In theory incorrect behaviour might be able to occur if the control key state was changed at the wrong instant.

@TrueBrain
Copy link
Member

I've got a commit for the modifier key race issue here: JGRennison/OpenTTD-patches@59daa57

In theory incorrect behaviour might be able to occur if the control key state was changed at the wrong instant.

Too slow :P I went for a slightly different approach to the problem, but looking back I am not sure I like mine. I think yours is easier .. well, except for the part where you move next_draw_tick, that really isn't needed :P

@JGRennison
Copy link
Contributor Author

Too slow :P I went for a slightly different approach to the problem, but looking back I am not sure I like mine. I think yours is easier .. well, except for the part where you move next_draw_tick, that really isn't needed :P

Strictly speaking, it is possible to race on _settings_client.gui.refresh_rate if GetDrawInterval is called without holding the lock.
Not that this would really be a big problem if it occured.

@TrueBrain
Copy link
Member

Strictly speaking, it is possible to race on _settings_client.gui.refresh_rate if GetDrawInterval is called without holding the lock.
Not that this would really be a big problem if it occured.

Fair. But currently I think this setting can only be changed from the draw-thread, as it is a GUI-only routine, not?

@TrueBrain
Copy link
Member

I think I fixed them all, at least, all the once I could easily find. After all my PRs are merged, we can do another round of testing to see if we can find any others, and update this issue accordingly :)

It is funny that on the surface some look like it is a bogus message, but if you look really close, you can find a path of things going terribly wrong :P Nice :D

@James103
Copy link
Contributor

Is this issue still relevant in OpenTTD 12.2? In other words, does compiling OpenTTD 12.2 with ThreadSanitizer and running give any (current/new) thread safety problems?

@JGRennison
Copy link
Contributor Author

The races listed here don't seem to be present any more, so this can be closed.

The remaining races within OpenTTD code seem to be sound/mixer related, and can be handled separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@TrueBrain @JGRennison @James103