gemrb ResourceManager::GetResource is not thread safe

Bug description

the audio thread calls this frequently. The main problem is this useCorrupt business. Is there any way we can just ax that? or factor it out into a non thread safe version?

I'm sure there are other issues as well. All I know is that audio periodically stops working until something kicks it again. I should have checked if the thread was deadlocked, but TSAN would have reported a deadlock I believe. All I got were reports about GetResource and things related to it.

GemRB version: master

Steps to reproduce

Build and run with TSAN

Expected behavior

Audio thread only calls APIs that are thread safe

Jun 19 '21 00:06 bradallred

Yeah, there may be a way to remove it, since there's only one user.

Jun 19 '21 08:06 lynxlynxlynx

Now that I've looked at the code, how would this affect audio? The hack is only used for PLT loading in one GUIScript binding and doesn't come into play for other uses.

Jul 21 '21 15:07 lynxlynxlynx

==================
WARNING: ThreadSanitizer: data race (pid=35737)
  Write of size 1 at 0x7ba40000eb11 by main thread:
    #0 GemRB::ResourceManager::GetResource(char const*, GemRB::TypeID const*, bool, bool) const ResourceManager.cpp:137 (libgemrb_core.dylib:x86_64+0x2a4ef9)
    #1 GemRB::Holder<GemRB::SoundMgr> GemRB::GetResourceHolder<GemRB::SoundMgr>(char const*, GemRB::ResourceManager const&, bool) GameData.h:183 (MUSImporter.so:x86_64+0x6eed)
    #2 GemRB::MUSImporter::PlayMusic(char*) MUSImporter.cpp:308 (MUSImporter.so:x86_64+0x6514)
    #3 GemRB::MUSImporter::PlayMusic(int) MUSImporter.cpp:290 (MUSImporter.so:x86_64+0x60fc)
    #4 GemRB::MUSImporter::Start() MUSImporter.cpp:196 (MUSImporter.so:x86_64+0x5e30)
    #5 GemRB::MUSImporter::SwitchPlayList(char const*, bool) MUSImporter.cpp:244 (MUSImporter.so:x86_64+0x69fa)
    #6 GemRB::Map::PlayAreaSong(int, bool, bool) const Map.cpp:2482 (libgemrb_core.dylib:x86_64+0x1e6c99)
    #7 GemRB::Game::ChangeSong(bool, bool) const Game.cpp:2158 (libgemrb_core.dylib:x86_64+0xb8928)
    #8 GemRB::Game::GetMap(char const*, bool) Game.cpp:740 (libgemrb_core.dylib:x86_64+0xb7f38)
    #9 GemRB::GameControl::ChangeMap(GemRB::Actor const*, bool) GameControl.cpp:2562 (libgemrb_core.dylib:x86_64+0x47dddc)
    #10 GemRB::Interface::HandleFlags() Interface.cpp:498 (libgemrb_core.dylib:x86_64+0x139397)
    #11 GemRB::Interface::Main() Interface.cpp:860 (libgemrb_core.dylib:x86_64+0x13e950)
    #12 -[CocoaWrapper launchGame:] CocoaWrapper.mm:204 (GemRB:x86_64+0x100004828)
    #13 __NSFirePerformWithOrder <null>:2 (Foundation:x86_64+0x12fa23)
    #14 start <null>:2 (libdyld.dylib:x86_64+0x15f5c)

  Previous write of size 1 at 0x7ba40000eb11 by thread T16 (mutexes: write M203641940149916032):
    #0 GemRB::ResourceManager::GetResource(char const*, GemRB::TypeID const*, bool, bool) const ResourceManager.cpp:137 (libgemrb_core.dylib:x86_64+0x2a4ef9)
    #1 GemRB::Holder<GemRB::SoundMgr> GemRB::GetResourceHolder<GemRB::SoundMgr>(char const*, bool, bool) GameData.h:177 (OpenALAudio.so:x86_64+0x14cb9)
    #2 GemRB::OpenALAudioDriver::loadSound(char const*, unsigned long&) OpenALAudio.cpp:404 (OpenALAudio.so:x86_64+0x14809)
    #3 GemRB::OpenALAudioDriver::QueueAmbient(int, char const*) OpenALAudio.cpp:789 (OpenALAudio.so:x86_64+0x17a76)
    #4 GemRB::AmbientMgrAL::AmbientSource::enqueue() const AmbientMgrAL.cpp:251 (OpenALAudio.so:x86_64+0x488e)
    #5 GemRB::AmbientMgrAL::AmbientSource::tick(unsigned long, GemRB::Point, unsigned int) AmbientMgrAL.cpp:237 (OpenALAudio.so:x86_64+0x3f04)
    #6 GemRB::AmbientMgrAL::tick(unsigned long) const AmbientMgrAL.cpp:134 (OpenALAudio.so:x86_64+0x30a1)
    #7 GemRB::AmbientMgrAL::play() AmbientMgrAL.cpp:110 (OpenALAudio.so:x86_64+0x13e5)
    #8 decltype(*(std::__1::forward<GemRB::AmbientMgrAL*>(fp0)).*fp()) std::__1::__invoke<int (GemRB::AmbientMgrAL::*)(), GemRB::AmbientMgrAL*, void>(int (GemRB::AmbientMgrAL::*&&)(), GemRB::AmbientMgrAL*&&) type_traits:3688 (OpenALAudio.so:x86_64+0xa6d7)
    #9 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, int (GemRB::AmbientMgrAL::*)(), GemRB::AmbientMgrAL*, 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, int (GemRB::AmbientMgrAL::*)(), GemRB::AmbientMgrAL*>&, std::__1::__tuple_indices<2ul>) thread:280 (OpenALAudio.so:x86_64+0xa491)
    #10 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, int (GemRB::AmbientMgrAL::*)(), GemRB::AmbientMgrAL*> >(void*) thread:291 (OpenALAudio.so:x86_64+0x93d9)

  Location is heap block of size 19232 at 0x7ba40000a000 allocated by main thread:
    #0 operator new(unsigned long) <null>:3 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x77fcb)
    #1 -[CocoaWrapper launchGame:] CocoaWrapper.mm:161 (GemRB:x86_64+0x100003a19)
    #2 __NSFirePerformWithOrder <null>:2 (Foundation:x86_64+0x12fa23)
    #3 start <null>:2 (libdyld.dylib:x86_64+0x15f5c)

  Mutex M203641940149916032 is already destroyed.

  Thread T16 (tid=527664, running) created by main thread at:
    #0 pthread_create <null>:3 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x2cd8d)
    #1 std::__1::__libcpp_thread_create(_opaque_pthread_t**, void* (*)(void*), void*) __threading_support:501 (OpenALAudio.so:x86_64+0x931e)
    #2 std::__1::thread::thread<int (GemRB::AmbientMgrAL::*)(), GemRB::AmbientMgrAL*, void>(int (GemRB::AmbientMgrAL::*&&)(), GemRB::AmbientMgrAL*&&) thread:307 (OpenALAudio.so:x86_64+0x8f2a)
    #3 std::__1::thread::thread<int (GemRB::AmbientMgrAL::*)(), GemRB::AmbientMgrAL*, void>(int (GemRB::AmbientMgrAL::*&&)(), GemRB::AmbientMgrAL*&&) thread:299 (OpenALAudio.so:x86_64+0x1558)
    #4 GemRB::AmbientMgrAL::AmbientMgrAL() AmbientMgrAL.cpp:40 (OpenALAudio.so:x86_64+0x10db)
    #5 GemRB::AmbientMgrAL::AmbientMgrAL() AmbientMgrAL.cpp:39 (OpenALAudio.so:x86_64+0x1798)
    #6 GemRB::OpenALAudioDriver::Init() OpenALAudio.cpp:253 (OpenALAudio.so:x86_64+0x12c10)
    #7 GemRB::Interface::Init(GemRB::InterfaceConfig*) Interface.cpp:1562 (libgemrb_core.dylib:x86_64+0x14807f)
    #8 -[CocoaWrapper launchGame:] CocoaWrapper.mm:195 (GemRB:x86_64+0x100004692)
    #9 __NSFirePerformWithOrder <null>:2 (Foundation:x86_64+0x12fa23)
    #10 start <null>:2 (libdyld.dylib:x86_64+0x15f5c)

SUMMARY: ThreadSanitizer: data race ResourceManager.cpp:137 in GemRB::ResourceManager::GetResource(char const*, GemRB::TypeID const*, bool, bool) const
==================

as you can see it is called by OpenALAudioDriver::loadSound on line 404

Jul 21 '21 15:07 bradallred

Would just marking core->UseCorruptedHack as thread_local do the trick? Doesn't look like it:

If the thread that executes the expression that accesses this object is not the thread that executed its initialization, the behavior is implementation-defined.

To remove the hack, we'd need some more methods, so we can differentiate missing and broken files. Can probably get internalized into GemRB_Button_SetPLT instead, so we don't need to bother.

Jul 21 '21 15:07 lynxlynxlynx

thread_local could work if it weren't a member of core. Of course this definition of "work" is kind of a hack in and of itself because it wouldn't work if UseCorruptedHack were needed for anything on another thread.

It would be much better to remove it (especially given that it is a hack)

Jul 21 '21 17:07 bradallred