ao_coreaudio: unregister hotplug listener when init fails - #18382
Open
SeanLF wants to merge 1 commit into
Open
Conversation
Comment on lines
+522
to
+532
| // Removing a listener that was never added is a no-op, so undo them all. | ||
| for (int i = 0; i < MP_ARRAY_SIZE(hotplug_properties); i++) { | ||
| AudioObjectPropertyAddress addr = { | ||
| hotplug_properties[i], | ||
| kAudioObjectPropertyScopeGlobal, | ||
| kAudioObjectPropertyElementMain | ||
| }; | ||
| AudioObjectRemovePropertyListener( | ||
| kAudioObjectSystemObject, &addr, hotplug_cb, (void *)ao); | ||
| } | ||
| p->hotplug_cb_registration_times--; |
Contributor
There was a problem hiding this comment.
why duplicate unregister_hotplug_cb here but not in init?
Member
|
Can we instead register callbacks after successful init, just move it down in the init() function? |
ao_uninit() calls the driver's uninit() only once ao->driver_initialized is set, i.e. after init() has succeeded, but it frees the ao either way. init() registered the hotplug listener before the steps that can fail, so a failed init left CoreAudio holding two property listeners whose clientData pointed at freed memory. mpv falls back to the next ao driver, so the process stays alive and the next device change enters hotplug_cb() on the freed ao. Easy to hit on macOS 26/27, where init_audiounit() fails with -50 for some channel layouts and mpv falls back to ao_avfoundation. Under ASan this is a heap-use-after-free in hotplug_cb() with the free attributed to ao_uninit(); without it the crash depends on what reuses the block, since mp_msg_level() dereferences log unconditionally. Register after everything that can fail instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SeanLF
force-pushed
the
ao-coreaudio-hotplug-uaf
branch
from
August 18, 2026 14:46
a714477 to
3138c93
Compare
Contributor
|
Okay, judging by these interactions you literally do not understand the code you're submitting, and now we're prompting Claude through you as a proxy. Cool |
Author
|
@kasper93 Fair call-out on the process. I applied the fix you requested (register after init) |
3 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ao_coreaudio'sinit()registers the hotplug listener withaoasclientData, then returnsCONTROL_ERRORon any later failure without unregistering it.ao_uninit()skipsdriver->uninit()for a failed init buttalloc_free()s theaoregardless, so CoreAudio keeps two listeners pointing at freed memory. mpv falls back to the next driver, so the process stays alive and the next device change entershotplug_cb()on the freedao.ao_coreaudio_exclusive.calready undoes its own listener at the equivalent error label; this does the same.Common on macOS 26/27, where
init_audiounit()fails with-50for some channel layouts and mpv falls back toao_avfoundation:Trigger a device change after that and, on an ASan build:
Clean with the patch;
coreaudiostill initialises and plays normally where it did before. Reported as #18274.Reproduction without audio hardware
Create and destroy an aggregate device to produce a
kAudioHardwarePropertyDeviceschange — no driver install or root needed:The device must not be private — a private aggregate device is visible only to the creating process and delivers no cross-process notification.
Play a file with a mono audio track (which makes
coreaudioinit fail), then run that. On a release build the crash is intermittent rather than reliable:mp_msg_level()dereferenceslogunconditionally, so it only faults when the freed block happens to be reused and zeroed, which is the null dereference inmp_msg_vafrom #18274. Under ASan it reproduces every time.Tested on macOS 27.0 (26A5416b), Apple M4 Pro.
Adjacent issues I left alone
--coreaudio-change-physical-format, a failure afterinit_physical_format()leaves the device on the changed format —p->original_asbdis only restored inuninit(). Same structural gap, one field over.p->audio_unitis never set to NULL afterAudioComponentInstanceDispose(), sohotplug_cb'sif (p->audio_unit)guard can callAudioUnitGetPropertyon a disposed instance. Returns an error rather than crashing on macOS 27.unregister_hotplug_cb(). Holding a listener inside a 1500 ms sleep,AudioObjectRemovePropertyListener()called from another thread blocked 1504-1505 ms across three runs and no callback fired after it returned, so I found no race to guard against here. Single-OS observation, not a documented guarantee.Not blocking this PR, just flagging it since it is the underlying cause: the rule that a failed
init()never getsuninit()is only implicit inao_uninit()andao_init(), andinternal.h's comment on.initdoes not mention it.ao_alsa,ao_pulse,ao_wasapiandao_coreaudio_exclusiveall clean up after themselves;ao_coreaudiowas the one that did not. Would you want a note on.initininternal.h, oruninit()called for failed inits too? Happy to send either as a separate PR.Assisted with Claude Code