Bug 172101 – Crash under WebCore::AudioSourceProviderAVFObjC::process().

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

WebCore::AudioSourceProviderAVFObjC::process() is calling MTAudioProcessingTapGetSourceAudio() where the value of the parameter MTAudioProcessingTapRef is NULL.

Crashed Thread:        29  AUDeferredRenderer-0x7fe86d4e20

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000010
Exception Note:        EXC_CORPSE_NOTIFY

Thread 29 Crashed:: AUDeferredRenderer-0x7fe86d4e20
0   com.apple.MediaToolbox        	0x7fffa8aabb53 MTAudioProcessingTapGetSourceAudio + 129 (/Library/Caches/com.apple.xbs/Sources/CoreMedia/CoreMedia-1873/Prototypes/MediaConverter/MTAudioProcessingTap.c:869)
1   com.apple.WebCore             	0x00007fffac05b580 WebCore::AudioSourceProviderAVFObjC::process(long, unsigned int, AudioBufferList*, long*, unsigned int*) + 64
2   com.apple.MediaToolbox        	0x7fffa8aacaab aptap_AudioQueueProcessingTapCallback + 158 (/Library/Caches/com.apple.xbs/Sources/CoreMedia/CoreMedia-1873/Prototypes/MediaConverter/MTAudioProcessingTap.c:466)
3   com.apple.audio.toolbox.AudioToolbox	0x00007fffa450c97f AQClientCallProcessingTap(void*, unsigned int, AudioTimeStamp*, unsigned int*, unsigned int*, AudioBufferList*, int, double, unsigned int, double, unsigned int, unsigned char const*, unsigned int) + 93
4   com.apple.audio.toolbox.AudioToolbox	0x00007fffa45934b0 AQProcessingTap::DoCallout(unsigned int&, AudioTimeStamp&, unsigned int&, AudioBufferList&) + 188
5   com.apple.audio.toolbox.AudioToolbox	0x00007fffa45932af AudioQueueObject::PerformTapInternal(AudioTimeStamp&, unsigned int&, unsigned int&) + 133
6   com.apple.audio.toolbox.AudioToolbox	0x00007fffa45931b5 AudioQueueObject::PerformProcessingTap(int (*)(void*, unsigned int&, AudioTimeStamp const&, unsigned int, AudioBufferList&, double&, unsigned int&), void*, AudioTimeStamp&, unsigned int&, AudioBufferList&, unsigned int&) + 197
7   com.apple.audio.toolbox.AudioToolbox	0x00007fffa459ddd9 MEMixerChannel::TapDownstream(void*, unsigned int*, AudioTimeStamp const*, unsigned int, unsigned int, AudioBufferList*) + 129
8   com.apple.audio.units.Components	0x000000011a51c6b5 AUInputElement::PullInput(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int) + 185
9   com.apple.audio.units.Components	0x000000011a45f374 AUDeferredRenderer::Produce() + 576
10  com.apple.audio.units.Components	0x000000011a45f0ab AUDeferredRenderer::ProducerThreadProc() + 49
11  com.apple.audio.units.Components	0x000000011a45eaab AUDeferredRenderer::ProducerThreadProc(void*) + 31
12  com.apple.audio.units.Components	0x000000011a532737 CAPThread::Entry(CAPThread*) + 85
13  libsystem_pthread.dylib       	0x00007fffb9accc4f _pthread_body + 176
14  libsystem_pthread.dylib       	0x00007fffb9accb9f _pthread_start + 274
15  libsystem_pthread.dylib       	0x00007fffb9acc3d5 thread_start + 13

[Per Arne Vollan]

<rdar://problem/27446589>

[Per Arne Vollan]

Created attachment 310115 [details]
Patch

[Brent Fulgham]

Comment on attachment 310115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310115&action=review

> Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:364
> +        return;

I wonder how we get into this state? We should probably ASSERT in "initCallback" if the passed 'tap' is nullptr, since I don't think this is supposed to happen.

It seems like this might happen if 'destroyMix()' is called, and then something attempts to use the AudioSourceProviderAVFObjC again (some queued 'process' callback gets invoked).

[Per Arne Vollan]

Created attachment 310250 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 310115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310115&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:364
> > +        return;
> 
> I wonder how we get into this state? We should probably ASSERT in
> "initCallback" if the passed 'tap' is nullptr, since I don't think this is
> supposed to happen.
> 
> It seems like this might happen if 'destroyMix()' is called, and then
> something attempts to use the AudioSourceProviderAVFObjC again (some queued
> 'process' callback gets invoked).

Thanks for reviewing! I have added an ASSERT in initCallback, and included a scenario of how this might happen in the changelog.

[Per Arne Vollan]

Created attachment 310251 [details]
Patch

[Brent Fulgham]

Comment on attachment 310251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310251&action=review

I think this looks fine, but I'd like to hear from Jer or Eric before landing.

> Source/WebCore/ChangeLog:19
> +        will crash.

Should we be asserting that the calls are being made on the main thread?

[Jer Noble]

Comment on attachment 310251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310251&action=review

>> Source/WebCore/ChangeLog:19
>> +        will crash.
> 
> Should we be asserting that the calls are being made on the main thread?

If it's possible to destroy the tap simultaneous to using it on separate threads, then this code might reduce NULL-deref crashes, but might still lead to calling into destroyed objects.

> Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:366
> +    if (!m_tap)
> +        return;
> +    

However, it should be possible to eliminate that possibility by doing something like:

RetainPtr<MTAudioProcessingTapRef> tap = m_tap;
if (!tap)
    return;

And then:

> Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:369
>      OSStatus status = MTAudioProcessingTapGetSourceAudio(m_tap.get(), numberOfFrames, bufferListInOut, flagsOut, &rangeOut, &itemCount);

Using tap instead of m_tap in this call.

This is an unfortunate amount of ref-count churn, but the only other solution here would be to add a bunch of locks, which have their own performance problems.

[Per Arne Vollan]

Created attachment 310267 [details]
Patch

[Per Arne Vollan]

(In reply to Jer Noble from comment #8)
> Comment on attachment 310251 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310251&action=review
> 
> >> Source/WebCore/ChangeLog:19
> >> +        will crash.
> > 
> > Should we be asserting that the calls are being made on the main thread?
> 
> If it's possible to destroy the tap simultaneous to using it on separate
> threads, then this code might reduce NULL-deref crashes, but might still
> lead to calling into destroyed objects.
> 
> > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:366
> > +    if (!m_tap)
> > +        return;
> > +    
> 
> However, it should be possible to eliminate that possibility by doing
> something like:
> 
> RetainPtr<MTAudioProcessingTapRef> tap = m_tap;

Do we still need locking to make sure this assignment is thread safe?

> if (!tap)
>     return;
> 
> And then:
> 
> > Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:369
> >      OSStatus status = MTAudioProcessingTapGetSourceAudio(m_tap.get(), numberOfFrames, bufferListInOut, flagsOut, &rangeOut, &itemCount);
> 
> Using tap instead of m_tap in this call.
> 
> This is an unfortunate amount of ref-count churn, but the only other
> solution here would be to add a bunch of locks, which have their own
> performance problems.

Thanks for reviewing! I updated the patch.

[Jer Noble]

View in context: https://bugs.webkit.org/attachment.cgi?id=310251&action=review

>>>> Source/WebCore/ChangeLog:19
>>>> +        will crash.
>>> 
>>> Should we be asserting that the calls are being made on the main thread?
>> 
>> If it's possible to destroy the tap simultaneous to using it on separate threads, then this code might reduce NULL-deref crashes, but might still lead to calling into destroyed objects.
> 
> Do we still need locking to make sure this assignment is thread safe?

I would file a follow-up bug to see if we need a blocking mechanism to ensure that the tap isn't freed while in the middle of a process(). We may need to add something.

[Per Arne Vollan]

(In reply to Jer Noble from comment #11)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310251&action=review
> 
> >>>> Source/WebCore/ChangeLog:19
> >>>> +        will crash.
> >>> 
> >>> Should we be asserting that the calls are being made on the main thread?
> >> 
> >> If it's possible to destroy the tap simultaneous to using it on separate threads, then this code might reduce NULL-deref crashes, but might still lead to calling into destroyed objects.
> > 
> > Do we still need locking to make sure this assignment is thread safe?
> 
> I would file a follow-up bug to see if we need a blocking mechanism to
> ensure that the tap isn't freed while in the middle of a process(). We may
> need to add something.

I see that the MTAudioProcessingTapRef is passed in the processCallback() function. What do you think about just passing this on to the process() method, and use it there instead of using m_tap? This parameter will not be null even if the main thread sets m_tap to null, and I assume it is retained by MediaToolbox, so it can be used safely?

[WebKit Commit Bot]

Comment on attachment 310267 [details]
Patch

Clearing flags on attachment: 310267

Committed r217005: <http://trac.webkit.org/changeset/217005>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.