geemili
/
SDL
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

audio: Protect against race conditions when closing a physical device. 

This specifically deals with two threads closing the same device at the same
time, and a thread trying to reopen the device as it's still in process of
being closed. This can happen in normal usage if a device is disconnected:
the OS might send a disconnect event, while the device thread also attempts
to manage a disconnect as system calls start to report failure.

This effort is necessary because we have to release the device lock during
close to allow the device thread to unblock and cleanly shutdown. But the
good news is that all the places that call ClosePhysicalAudioDevice can now
safely hold the device lock on entry, and that one function will manage the
lock tapdancing.
main
Ryan C. Gordon 2023-11-15 14:12:31 -05:00
parent 8fa0746d4a
commit e923a458ea
No known key found for this signature in database
GPG Key ID: FA148B892AB48044
2 changed files with 50 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
src/audio/SDL_audio.c
View File

@ -466,14 +466,15 @@ static void DestroyPhysicalAudioDevice(SDL_AudioDevice *device)
while (device->logical_devices) {
DestroyLogicalAudioDevice(device->logical_devices);
}
SDL_UnlockMutex(device->lock); // don't use ReleaseAudioDevice because we don't want to change refcounts while destroying.
// it's safe to not hold the lock for this (we can't anyhow, or the audio thread won't quit), because we shouldn't be in the device list at this point.
ClosePhysicalAudioDevice(device);
current_audio.impl.FreeDeviceHandle(device);
SDL_UnlockMutex(device->lock); // don't use ReleaseAudioDevice because we don't want to change refcounts while destroying.
SDL_DestroyMutex(device->lock);
SDL_DestroyCondition(device->close_cond);
SDL_free(device->work_buffer);
SDL_free(device->name);
SDL_free(device);
@ -529,6 +530,14 @@ static SDL_AudioDevice *CreatePhysicalAudioDevice(const char *name, SDL_bool isc
return NULL;
}
device->close_cond = SDL_CreateCondition();
if (!device->close_cond) {
SDL_DestroyMutex(device->lock);
SDL_free(device->name);
SDL_free(device);
return NULL;
}
SDL_AtomicSet(&device->shutdown, 0);
SDL_AtomicSet(&device->zombie, 0);
device->iscapture = iscapture;
@ -1380,10 +1389,29 @@ int SDL_GetAudioDeviceFormat(SDL_AudioDeviceID devid, SDL_AudioSpec *spec, int *
return retval;
}
// this expects the device lock to be held. !!! FIXME: no it doesn't...?
// this is awkward, but this makes sure we can release the device lock
// so the device thread can terminate but also not have two things
// race to close or open the device while the lock is unprotected.
// you hold the lock when calling this, it will release the lock and
// wait while the shutdown flag is set.
// BE CAREFUL WITH THIS.
static void SerializePhysicalDeviceClose(SDL_AudioDevice *device)
{
while (SDL_AtomicGet(&device->shutdown)) {
SDL_WaitCondition(device->close_cond, device->lock);
}
}
// this expects the device lock to be held.
static void ClosePhysicalAudioDevice(SDL_AudioDevice *device)
{
SerializePhysicalDeviceClose(device);
SDL_AtomicSet(&device->shutdown, 1);
// YOU MUST PROTECT KEY POINTS WITH SerializePhysicalDeviceClose() WHILE THE THREAD JOINS
SDL_UnlockMutex(device->lock);
if (device->thread) {
SDL_WaitThread(device->thread, NULL);
device->thread = NULL;
@ -1395,6 +1423,10 @@ static void ClosePhysicalAudioDevice(SDL_AudioDevice *device)
device->hidden = NULL; // just in case.
}
SDL_LockMutex(device->lock);
SDL_AtomicSet(&device->shutdown, 0); // ready to go again.
SDL_BroadcastCondition(device->close_cond); // release anyone waiting in SerializePhysicalDeviceClose; they'll still block until we release device->lock, though.
SDL_aligned_free(device->work_buffer);
device->work_buffer = NULL;
@ -1407,28 +1439,24 @@ static void ClosePhysicalAudioDevice(SDL_AudioDevice *device)
SDL_copyp(&device->spec, &device->default_spec);
device->sample_frames = 0;
device->silence_value = SDL_GetSilenceValueForFormat(device->spec.format);
SDL_AtomicSet(&device->shutdown, 0); // ready to go again.
}
void SDL_CloseAudioDevice(SDL_AudioDeviceID devid)
{
SDL_bool close_physical = SDL_FALSE;
SDL_AudioDevice *device = NULL;
SDL_LogicalAudioDevice *logdev = ObtainLogicalAudioDevice(devid, &device);
if (logdev) {
DestroyLogicalAudioDevice(logdev);
close_physical = (!device->logical_devices); // no more logical devices? Close the physical device, too.
}
// !!! FIXME: we _need_ to release this lock, but doing so can cause a race condition if someone opens a device while we're closing it.
ReleaseAudioDevice(device); // can't hold the lock or the audio thread will deadlock while we WaitThread it. If not closing, we're done anyhow.
if (device) {
if (close_physical) {
if (!device->logical_devices) { // no more logical devices? Close the physical device, too.
ClosePhysicalAudioDevice(device);
}
UnrefPhysicalAudioDevice(device); // one reference for each logical device.
}
ReleaseAudioDevice(device);
}
@ -1501,8 +1529,11 @@ char *SDL_GetAudioThreadName(SDL_AudioDevice *device, char *buf, size_t buflen)
// this expects the device lock to be held.
static int OpenPhysicalAudioDevice(SDL_AudioDevice *device, const SDL_AudioSpec *inspec)
{
SDL_assert(!device->currently_opened);
SDL_assert(!device->logical_devices);
SerializePhysicalDeviceClose(device); // make sure another thread that's closing didn't release the lock to let the device thread join...
if (device->currently_opened) {
return 0; // we're already good.
}
// Just pretend to open a zombie device. It can still collect logical devices on a default device under the assumption they will all migrate when the default device is officially changed.
if (SDL_AtomicGet(&device->zombie)) {
@ -1600,7 +1631,7 @@ SDL_AudioDeviceID SDL_OpenAudioDevice(SDL_AudioDeviceID devid, const SDL_AudioSp
SDL_SetError("Device was already lost and can't accept new opens");
} else if ((logdev = (SDL_LogicalAudioDevice *) SDL_calloc(1, sizeof (SDL_LogicalAudioDevice))) == NULL) {
SDL_OutOfMemory();
} else if (!device->currently_opened && OpenPhysicalAudioDevice(device, spec) == -1) { // first thing using this physical device? Open at the OS level...
} else if (OpenPhysicalAudioDevice(device, spec) == -1) { // if this is the first thing using this physical device, open at the OS level if necessary...
SDL_free(logdev);
} else {
RefPhysicalAudioDevice(device); // unref'd on successful SDL_CloseAudioDevice
@ -2026,10 +2057,9 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device)
}
if (needs_migration) {
if (!new_default_device->currently_opened) { // New default physical device not been opened yet? Open at the OS level...
if (OpenPhysicalAudioDevice(new_default_device, &spec) == -1) {
needs_migration = SDL_FALSE; // uhoh, just leave everything on the old default, nothing to be done.
}
// New default physical device not been opened yet? Open at the OS level...
if (OpenPhysicalAudioDevice(new_default_device, &spec) == -1) {
needs_migration = SDL_FALSE; // uhoh, just leave everything on the old default, nothing to be done.
}
}
@ -2086,12 +2116,7 @@ void SDL_DefaultAudioDeviceChanged(SDL_AudioDevice *new_default_device)
UpdateAudioStreamFormatsPhysical(new_default_device);
if (!current_default_device->logical_devices) { // nothing left on the current physical device, close it.
// !!! FIXME: we _need_ to release this lock, but doing so can cause a race condition if someone opens a device while we're closing it.
RefPhysicalAudioDevice(current_default_device); // hold a temp ref for a moment while we release...
ReleaseAudioDevice(current_default_device); // can't hold the lock or the audio thread will deadlock while we WaitThread it.
ClosePhysicalAudioDevice(current_default_device);
ObtainPhysicalAudioDeviceObj(current_default_device); // we're about to unlock this again, so make sure the locks match.
UnrefPhysicalAudioDevice(current_default_device); // drop temp ref.
}
}

3
src/audio/SDL_sysaudio.h
View File

@ -257,6 +257,9 @@ struct SDL_AudioDevice
// A mutex for locking access to this struct
SDL_Mutex *lock;
// A condition variable to protect device close, where we can't hold the device lock forever.
SDL_Condition *close_cond;
// Reference count of the device; logical devices, device threads, etc, add to this.
SDL_AtomicInt refcount;