-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
gstreamer: fix crash in gst_osx_audio_src_io_proc when starting VM
Fixes #4629
- Loading branch information
Showing
1 changed file
with
298 additions
and
14 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,308 @@ | ||
| diff -Naur a/sys/osxaudio/gstosxcoreaudiocommon.c b/sys/osxaudio/gstosxcoreaudiocommon.c | ||
| --- a/sys/osxaudio/gstosxcoreaudiocommon.c 2021-05-31 16:11:47.000000000 -0700 | ||
| +++ b/sys/osxaudio/gstosxcoreaudiocommon.c 2022-12-24 23:00:48.000000000 -0800 | ||
| @@ -137,15 +137,16 @@ | ||
| "osx ring buffer stop ioproc: %p device_id %lu", | ||
| core_audio->element->io_proc, (gulong) core_audio->device_id); | ||
| From 3a6ea13fd15abffea92fb866a6d2e8905a8a2f4f Mon Sep 17 00:00:00 2001 | ||
| From: osy <osy@turing.llc> | ||
| Date: Sat, 4 Mar 2023 20:52:34 -0800 | ||
| Subject: [PATCH] osxaudio: fix race condition when removing AU callback | ||
|
|
||
| If the IO thread has already entered the callback when it is being removed, | ||
| the callback function can use memory that is being freed. We address this by | ||
| wrapping the callback function with something that holds a lock. | ||
| --- | ||
| sys/osxaudio/gstosxcoreaudio.c | 18 +++++++++++ | ||
| sys/osxaudio/gstosxcoreaudio.h | 1 + | ||
| sys/osxaudio/gstosxcoreaudiocommon.c | 41 +++++++++++++++++++++++--- | ||
| sys/osxaudio/gstosxcoreaudiocommon.h | 9 ------ | ||
| sys/osxaudio/gstosxcoreaudiohal.c | 10 +++++++ | ||
| sys/osxaudio/gstosxcoreaudioremoteio.c | 2 ++ | ||
| 6 files changed, 68 insertions(+), 13 deletions(-) | ||
|
|
||
| diff --git a/sys/osxaudio/gstosxcoreaudio.c b/sys/osxaudio/gstosxcoreaudio.c | ||
| index c432ee630..4fae6aff3 100644 | ||
| --- a/sys/osxaudio/gstosxcoreaudio.c | ||
| +++ b/sys/osxaudio/gstosxcoreaudio.c | ||
| @@ -27,6 +27,7 @@ | ||
| GST_DEBUG_CATEGORY_STATIC (osx_audio_debug); | ||
| #define GST_CAT_DEFAULT osx_audio_debug | ||
|
|
||
| +#define gst_core_audio_parent_class parent_class | ||
| G_DEFINE_TYPE (GstCoreAudio, gst_core_audio, G_TYPE_OBJECT); | ||
|
|
||
| #ifdef HAVE_IOS | ||
| @@ -35,10 +36,26 @@ G_DEFINE_TYPE (GstCoreAudio, gst_core_audio, G_TYPE_OBJECT); | ||
| #include "gstosxcoreaudiohal.c" | ||
| #endif | ||
|
|
||
| +static void | ||
| +gst_core_audio_finalize (GObject * object) | ||
| +{ | ||
| + GstCoreAudio *core_audio; | ||
| + | ||
| + core_audio = GST_CORE_AUDIO (object); | ||
| + | ||
| + g_mutex_clear (&core_audio->io_proc_lock); | ||
| + | ||
| + G_OBJECT_CLASS (parent_class)->finalize (object); | ||
| +} | ||
|
|
||
| static void | ||
| gst_core_audio_class_init (GstCoreAudioClass * klass) | ||
| { | ||
| + GObjectClass *gobject_class; | ||
| + | ||
| + gobject_class = (GObjectClass *) klass; | ||
| + parent_class = g_type_class_peek_parent (klass); | ||
| + gobject_class->finalize = gst_core_audio_finalize; | ||
| } | ||
|
|
||
| static void | ||
| @@ -54,6 +71,7 @@ gst_core_audio_init (GstCoreAudio * core_audio) | ||
| core_audio->hog_pid = -1; | ||
| core_audio->disabled_mixing = FALSE; | ||
| #endif | ||
| + g_mutex_init (&core_audio->io_proc_lock); | ||
| } | ||
|
|
||
| static gboolean | ||
| diff --git a/sys/osxaudio/gstosxcoreaudio.h b/sys/osxaudio/gstosxcoreaudio.h | ||
| index ee88e3cf4..8d3f91fa7 100644 | ||
| --- a/sys/osxaudio/gstosxcoreaudio.h | ||
| +++ b/sys/osxaudio/gstosxcoreaudio.h | ||
| @@ -93,6 +93,7 @@ struct _GstCoreAudio | ||
| gint stream_idx; | ||
| gboolean io_proc_active; | ||
| gboolean io_proc_needs_deactivation; | ||
| + GMutex io_proc_lock; | ||
|
|
||
| /* For LPCM in/out */ | ||
| AudioUnit audiounit; | ||
| diff --git a/sys/osxaudio/gstosxcoreaudiocommon.c b/sys/osxaudio/gstosxcoreaudiocommon.c | ||
| index 39d03ac5b..0200cbfbc 100644 | ||
| --- a/sys/osxaudio/gstosxcoreaudiocommon.c | ||
| +++ b/sys/osxaudio/gstosxcoreaudiocommon.c | ||
| @@ -23,7 +23,17 @@ | ||
|
|
||
| + // ###: why is it okay to directly remove from here but not from pause() ? | ||
| #include "gstosxcoreaudiocommon.h" | ||
|
|
||
| -void | ||
| +static OSStatus gst_core_audio_render_notify (GstCoreAudio * core_audio, | ||
| + AudioUnitRenderActionFlags * ioActionFlags, | ||
| + const AudioTimeStamp * inTimeStamp, | ||
| + unsigned int inBusNumber, | ||
| + unsigned int inNumberFrames, | ||
| + AudioBufferList * ioData); | ||
| + | ||
| +/** | ||
| + * core_audio->io_proc_lock must be held before calling! | ||
| +*/ | ||
| +static void | ||
| gst_core_audio_remove_render_callback (GstCoreAudio * core_audio) | ||
| { | ||
| AURenderCallbackStruct input; | ||
| @@ -57,7 +67,7 @@ gst_core_audio_remove_render_callback (GstCoreAudio * core_audio) | ||
| core_audio->io_proc_active = FALSE; | ||
| } | ||
|
|
||
| -OSStatus | ||
| +static OSStatus | ||
| gst_core_audio_render_notify (GstCoreAudio * core_audio, | ||
| AudioUnitRenderActionFlags * ioActionFlags, | ||
| const AudioTimeStamp * inTimeStamp, | ||
| @@ -71,6 +81,7 @@ gst_core_audio_render_notify (GstCoreAudio * core_audio, | ||
| * work around some thread-safety issues in CoreAudio | ||
| */ | ||
| if ((*ioActionFlags) & kAudioUnitRenderAction_PreRender) { | ||
| + // core_audio->io_proc_lock held before call to AudioUnitRender | ||
| if (core_audio->io_proc_needs_deactivation) { | ||
| gst_core_audio_remove_render_callback (core_audio); | ||
| } | ||
| @@ -79,6 +90,22 @@ gst_core_audio_render_notify (GstCoreAudio * core_audio, | ||
| return noErr; | ||
| } | ||
|
|
||
| +static OSStatus | ||
| +gst_core_audio_io_proc_callback (GstCoreAudio * core_audio, | ||
| + AudioUnitRenderActionFlags * ioActionFlags, | ||
| + const AudioTimeStamp * inTimeStamp, | ||
| + UInt32 inBusNumber, UInt32 inNumberFrames, AudioBufferList * bufferList) | ||
| +{ | ||
| + OSStatus status = 0; | ||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| + if (core_audio->io_proc_active) { | ||
| + gst_core_audio_remove_render_callback (core_audio); | ||
| + status = core_audio->element->io_proc (core_audio->osxbuf, ioActionFlags, inTimeStamp, inBusNumber, inNumberFrames, bufferList); | ||
| + } | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| + | ||
| + return status; | ||
| +} | ||
| + | ||
| status = AudioOutputUnitStop (core_audio->audiounit); | ||
| gboolean | ||
| gst_core_audio_io_proc_start (GstCoreAudio * core_audio) | ||
| { | ||
| @@ -86,6 +113,7 @@ gst_core_audio_io_proc_start (GstCoreAudio * core_audio) | ||
| AURenderCallbackStruct input; | ||
| AudioUnitPropertyID callback_type; | ||
|
|
||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| GST_DEBUG_OBJECT (core_audio->osxbuf, | ||
| "osx ring buffer start ioproc: %p device_id %lu", | ||
| core_audio->element->io_proc, (gulong) core_audio->device_id); | ||
| @@ -94,8 +122,8 @@ gst_core_audio_io_proc_start (GstCoreAudio * core_audio) | ||
| kAudioOutputUnitProperty_SetInputCallback : | ||
| kAudioUnitProperty_SetRenderCallback; | ||
|
|
||
| - input.inputProc = (AURenderCallback) core_audio->element->io_proc; | ||
| - input.inputProcRefCon = core_audio->osxbuf; | ||
| + input.inputProc = (AURenderCallback) gst_core_audio_io_proc_callback; | ||
| + input.inputProcRefCon = core_audio; | ||
|
|
||
| status = AudioUnitSetProperty (core_audio->audiounit, callback_type, kAudioUnitScope_Global, 0, /* N/A for global */ | ||
| &input, sizeof (input)); | ||
| @@ -103,6 +131,7 @@ gst_core_audio_io_proc_start (GstCoreAudio * core_audio) | ||
| if (status) { | ||
| GST_ERROR_OBJECT (core_audio->osxbuf, | ||
| "AudioUnitSetProperty failed: %d", (int) status); | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| return FALSE; | ||
| } | ||
| // ### does it make sense to do this notify stuff for input mode? | ||
| @@ -112,12 +141,14 @@ gst_core_audio_io_proc_start (GstCoreAudio * core_audio) | ||
| if (status) { | ||
| GST_ERROR_OBJECT (core_audio->osxbuf, | ||
| "AudioUnitAddRenderNotify failed %d", (int) status); | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| return FALSE; | ||
| } | ||
| core_audio->io_proc_active = TRUE; | ||
| } | ||
|
|
||
| core_audio->io_proc_needs_deactivation = FALSE; | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
|
|
||
| status = AudioOutputUnitStart (core_audio->audiounit); | ||
| if (status) { | ||
| GST_WARNING_OBJECT (core_audio->osxbuf, | ||
| @@ -143,9 +174,11 @@ gst_core_audio_io_proc_stop (GstCoreAudio * core_audio) | ||
| "AudioOutputUnitStop failed: %d", (int) status); | ||
| } | ||
| - // ###: why is it okay to directly remove from here but not from pause() ? | ||
| - if (core_audio->io_proc_active) { | ||
| - gst_core_audio_remove_render_callback (core_audio); | ||
| - } | ||
| // ###: why is it okay to directly remove from here but not from pause() ? | ||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| if (core_audio->io_proc_active) { | ||
| gst_core_audio_remove_render_callback (core_audio); | ||
| } | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| return TRUE; | ||
| } | ||
|
|
||
| diff --git a/sys/osxaudio/gstosxcoreaudiocommon.h b/sys/osxaudio/gstosxcoreaudiocommon.h | ||
| index c4602a6b3..a76b66656 100644 | ||
| --- a/sys/osxaudio/gstosxcoreaudiocommon.h | ||
| +++ b/sys/osxaudio/gstosxcoreaudiocommon.h | ||
| @@ -34,8 +34,6 @@ gboolean gst_core_audio_bind_device (GstCoreAudio *core_au | ||
|
|
||
| void gst_core_audio_dump_channel_layout (AudioChannelLayout * channel_layout); | ||
|
|
||
| -void gst_core_audio_remove_render_callback (GstCoreAudio * core_audio); | ||
| - | ||
| gboolean gst_core_audio_io_proc_start (GstCoreAudio * core_audio); | ||
|
|
||
| gboolean gst_core_audio_io_proc_stop (GstCoreAudio * core_audio); | ||
| @@ -54,13 +52,6 @@ gboolean gst_core_audio_open_device (GstCoreAudio *core_au | ||
| OSType sub_type, | ||
| const gchar *adesc); | ||
|
|
||
| -OSStatus gst_core_audio_render_notify (GstCoreAudio * core_audio, | ||
| - AudioUnitRenderActionFlags * ioActionFlags, | ||
| - const AudioTimeStamp * inTimeStamp, | ||
| - unsigned int inBusNumber, | ||
| - unsigned int inNumberFrames, | ||
| - AudioBufferList * ioData); | ||
| - | ||
| AudioChannelLabel gst_audio_channel_position_to_core_audio (GstAudioChannelPosition position, int channel); | ||
|
|
||
| GstAudioChannelPosition gst_core_audio_channel_label_to_gst (AudioChannelLabel label, int channel, gboolean warn); | ||
| diff --git a/sys/osxaudio/gstosxcoreaudiohal.c b/sys/osxaudio/gstosxcoreaudiohal.c | ||
| index f649e4fc7..79fed0d97 100644 | ||
| --- a/sys/osxaudio/gstosxcoreaudiohal.c | ||
| +++ b/sys/osxaudio/gstosxcoreaudiohal.c | ||
| @@ -917,6 +917,9 @@ done: | ||
| return ret; | ||
| } | ||
|
|
||
| +/** | ||
| + * core_audio->io_proc_lock must be held! | ||
| +*/ | ||
| static inline void | ||
| _remove_render_spdif_callback (GstCoreAudio * core_audio) | ||
| { | ||
| @@ -950,6 +953,7 @@ _io_proc_spdif_start (GstCoreAudio * core_audio) | ||
| "osx ring buffer start ioproc ID: %p device_id %lu", | ||
| core_audio->procID, (gulong) core_audio->device_id); | ||
|
|
||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| if (!core_audio->io_proc_active) { | ||
| /* Add IOProc callback */ | ||
| status = AudioDeviceCreateIOProcID (core_audio->device_id, | ||
| @@ -958,12 +962,14 @@ _io_proc_spdif_start (GstCoreAudio * core_audio) | ||
| if (status != noErr) { | ||
| GST_ERROR_OBJECT (core_audio->osxbuf, | ||
| ":AudioDeviceCreateIOProcID failed: %d", (int) status); | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| return FALSE; | ||
| } | ||
| core_audio->io_proc_active = TRUE; | ||
| } | ||
|
|
||
| core_audio->io_proc_needs_deactivation = FALSE; | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
|
|
||
| /* Start device */ | ||
| status = AudioDeviceStart (core_audio->device_id, core_audio->procID); | ||
| @@ -991,9 +997,11 @@ _io_proc_spdif_stop (GstCoreAudio * core_audio) | ||
| "osx ring buffer stop ioproc ID: %p device_id %lu", | ||
| core_audio->procID, (gulong) core_audio->device_id); | ||
|
|
||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| if (core_audio->io_proc_active) { | ||
| _remove_render_spdif_callback (core_audio); | ||
| } | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
|
|
||
| _close_spdif (core_audio); | ||
|
|
||
| @@ -1054,6 +1062,7 @@ gst_core_audio_start_processing_impl (GstCoreAudio * core_audio) | ||
| static gboolean | ||
| gst_core_audio_pause_processing_impl (GstCoreAudio * core_audio) | ||
| { | ||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| if (core_audio->is_passthrough) { | ||
| GST_DEBUG_OBJECT (core_audio, | ||
| "osx ring buffer pause ioproc ID: %p device_id %lu", | ||
| @@ -1074,6 +1083,7 @@ gst_core_audio_pause_processing_impl (GstCoreAudio * core_audio) | ||
| core_audio->io_proc_needs_deactivation = TRUE; | ||
| } | ||
| } | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| return TRUE; | ||
| } | ||
|
|
||
| diff --git a/sys/osxaudio/gstosxcoreaudioremoteio.c b/sys/osxaudio/gstosxcoreaudioremoteio.c | ||
| index 76b0eba32..10f5b18e4 100644 | ||
| --- a/sys/osxaudio/gstosxcoreaudioremoteio.c | ||
| +++ b/sys/osxaudio/gstosxcoreaudioremoteio.c | ||
| @@ -38,6 +38,7 @@ gst_core_audio_start_processing_impl (GstCoreAudio * core_audio) | ||
| static gboolean | ||
| gst_core_audio_pause_processing_impl (GstCoreAudio * core_audio) | ||
| { | ||
| + g_mutex_lock (&core_audio->io_proc_lock); | ||
| GST_DEBUG_OBJECT (core_audio, | ||
| "osx ring buffer pause ioproc: %p device_id %lu", | ||
| core_audio->element->io_proc, (gulong) core_audio->device_id); | ||
| @@ -49,6 +50,7 @@ gst_core_audio_pause_processing_impl (GstCoreAudio * core_audio) | ||
| */ | ||
| core_audio->io_proc_needs_deactivation = TRUE; | ||
| } | ||
| + g_mutex_unlock (&core_audio->io_proc_lock); | ||
| return TRUE; | ||
| } | ||
|
|
||
| -- | ||
| 2.37.1 (Apple Git-137.1) | ||
|
|