Data race in pulse audio initialization

[rom1v]

I reuse the sample from #7367: https://github.com/rom1v/stacksize_fail

meson setup t -Db_sanitize=thread
ninja -Ct
t/sample

==================
WARNING: ThreadSanitizer: data race (pid=1667598)
  Write of size 8 at 0x7b280003d7d8 by main thread (mutexes: write M538):
    #0 recvmsg ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:3062 (libtsan.so.2+0x589fe)
    #1 pa_iochannel_read_with_ancil_data <null> (libpulsecommon-16.1.so+0x328a7)
    #2 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x27189)

  Previous write of size 8 at 0x7b280003d7d8 by thread T8:
    #0 recvmsg ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:3062 (libtsan.so.2+0x589fe)
    #1 pa_iochannel_read_with_ancil_data <null> (libpulsecommon-16.1.so+0x328a7)

  Location is heap block of size 152 at 0x7b280003d7c0 allocated by main thread:
    #0 malloc ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:647 (libtsan.so.2+0x3ebb8)
    #1 pa_xmalloc <null> (libpulse.so.0+0x3a956)
    #2 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x27189)

  Mutex M538 (0x7b0c000011a0) created at:
    #0 pthread_mutex_init ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1295 (libtsan.so.2+0x50468)
    #1 <null> <null> (libSDL2-2.0.so.0+0x144191)
    #2 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x27189)

  Thread T8 'PulseHotplug' (tid=1667607, running) created by main thread at:
    #0 pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x5e686)
    #1 <null> <null> (libSDL2-2.0.so.0+0x143aba)
    #2 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x27189)

SUMMARY: ThreadSanitizer: data race (/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-16.1.so+0x328a7) in pa_iochannel_read_with_ancil_data
==================

The hotplug thread initializes things after init returns, so it is racy with SDL_OpenAudioDevice().

[icculus]

This appears to be a race condition in pulseaudio, not SDL, where it's reusing a socket for multiple contexts but not protecting it.

I'm hesitant to change this in SDL; we can't just slap a mutex around it, because the thread blocks inside pa_mainloop_run for its whole lifetime.

[icculus]

we can't just slap a mutex around it, because the thread blocks inside pa_mainloop_run for its whole lifetime.

So this is the code in PulseAudio itself for pa_mainloop_run ...

int pa_mainloop_run(pa_mainloop *m, int *retval) {
    int r;

    while ((r = pa_mainloop_iterate(m, 1, retval)) >= 0)
        ;

    if (r == -2)
        return 1;
    else
        return -1;
}

...honestly, let's just iterate in a loop ourselves and slap a mutex around it.

[slouken]

Sounds great. :)

[icculus]

Of course that second parameter to pa_mainloop_iterate tells it to block until something happens, and all our code relies on this, so we can't just slap a mutex around it...and that code is extremely sensitive to scheduling problems if we were to change this to non-blocking in a loop. :/

Now leaning towards closing this as WONTFIX. I'll think on it a bit more first, though.

[icculus]

PulseAudio has something called a Threaded Main Loop which is probably a more correct solution here, but it's going to take some redesign to incorporate it, so this might not close as WONTFIX, but rather slide into SDL3. I'll read up on this on Monday and see if it's a big change.

https://freedesktop.org/software/pulseaudio/doxygen/threaded_mainloop.html

[icculus]

So #7734 is kinda a big change for SDL2 when no one is having any real-world problems, so I have it sitting in a PR for SDL3 right now. I think it does improve the code in general and fixes this issue specifically, so I think it's a good change.

The question is whether we want to cherry-pick it back to SDL2 once it lands in SDL3.

Also note that there is still one ThreadSanitizer complaint remaining after this PR: it worries that, during device open, we lock the device detection lock and then the pa_threaded_mainloop lock in a different order than the hotplug thread does, which could deadlock. I'm still looking into this, and the SDL3 audio rewrite will likely eliminate it anyhow, since opening a device doesn't need the same locking as SDL2.

[icculus]

Threaded mainloop is in SDL3 now; do we want to cherry-pick this back to SDL2?

[slouken]

This is a big enough change I'm inclined to let it marinate in SDL 3 for a bit.

[icculus]

I'm bumping this out of the 2.28.0 milestone, it's shipping too soon to put this large a change in now. I'll put it in 2.30.0, or it might end up in 2.28.x if we decide that makes more sense.

[slouken]

Definitely not in 2.28.x, but 2.30.0 sounds good.

[icculus]

The threaded mainloop code is in for SDL2 (and has been in SDL3 for awhile now).

There is still one pending ThreadSanitizer warning, but I'm not confident it's a problem in real life. I have to dig a bit more on that before I close this bug for good.

[sezero]

Is this fixed already?

[icculus]

The ThreadSanitizer warning? No, not yet.

[icculus]

e51760e11166a81dd4ba504df559b9b16100815c fixes one of the two remaining ThreadSanitizer issues. This code changed for SDL3, so we don't need to cherry-pick it.

[icculus]

c331b64d796d2abf2fd03ec72f733642483d3623 resolves the rest of the ThreadSanitizer complaints for PulseAudio on SDL2.

SDL3 was already ThreadSanitizer-clean, and none of this code applies there...so we're done!