Bug 159238 – Throw exceptions for invalid number of channels for ConvolverNode

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

[David Kilzer (:ddkilzer)]

The ConvolverNode only supports 1, 2 or 4 channels and must throw an exception for any other number of channels.

[David Kilzer (:ddkilzer)]

This causes a heap-buffer-overflow read in com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::AudioArray<float>::copyToRange + 76 under ASan.

Application Specific Information:
================================================================
==30955==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200030a3f4 at pc 0x00010e94734a bp 0x7fff5360ed70 sp 0x7fff5360e528
READ of size 256 at 0x60200030a3f4 thread T0
    #0 0x10e947349 in __asan_memcpy (/Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.11.xctoolchain/usr/lib/clang/7.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x39349)
    #1 0x11398935b in WebCore::AudioArray<float>::copyToRange(float const*, unsigned int, unsigned int) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x97f35b)
    #2 0x11503b0bf in WebCore::ReverbConvolverStage::ReverbConvolverStage(float const*, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, WebCore::ReverbAccumulationBuffer*, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x20310bf)
    #3 0x11503b4b8 in WebCore::ReverbConvolverStage::ReverbConvolverStage(float const*, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, WebCore::ReverbAccumulationBuffer*, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x20314b8)
    #4 0x11503921a in WebCore::ReverbConvolver::ReverbConvolver(WebCore::AudioChannel*, unsigned long, unsigned long, unsigned long, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202f21a)
    #5 0x115037bba in WebCore::Reverb::initialize(WebCore::AudioBus*, unsigned long, unsigned long, unsigned long, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202dbba)
    #6 0x1150376cc in WebCore::Reverb::Reverb(WebCore::AudioBus*, unsigned long, unsigned long, unsigned long, bool, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202d6cc)
    #7 0x115037d57 in WebCore::Reverb::Reverb(WebCore::AudioBus*, unsigned long, unsigned long, unsigned long, bool, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202dd57)
    #8 0x1133ca260 in WebCore::ConvolverNode::setBuffer(WebCore::AudioBuffer*, int&) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x3c0260)
    #9 0x114073996 in WebCore::setJSConvolverNodeBuffer(JSC::ExecState*, long long, long long) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x1069996)
    #10 0x110359368 in JSC::callCustomSetter(JSC::ExecState*, bool (*)(JSC::ExecState*, long long, long long), bool, JSC::JSValue, JSC::JSValue) (/Users/local/OpenSource/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x574368)
[...]

[David Kilzer (:ddkilzer)]

<rdar://problem/27020410>

[David Kilzer (:ddkilzer)]

Created attachment 282306 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Please note that the patch for Bug 159232 needs to land before this one since it adds support to use anonymous functions and descriptive messages with shouldNotThrow() and shouldThrow().

[Brent Fulgham]

Comment on attachment 282306 [details]
Patch v1

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

Looks good. You'll need to land this manually, since the commit-queue doesn't process security bugs.

> Source/WebCore/ChangeLog:4
> +        <https://webkit.org/b/159238>

<rdar://problem/27020410>

> LayoutTests/ChangeLog:4
> +        <https://webkit.org/b/159238>

<rdar://problem/27020410>

> LayoutTests/webaudio/convolver-channels.html:18
> +    for (var count = 1; count <= 32; ++count) {

Just curious: Is 32 an arbitrary value? Or do we have Audio elements that use 32 channels?

[David Kilzer (:ddkilzer)]

Comment on attachment 282306 [details]
Patch v1

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

>> LayoutTests/webaudio/convolver-channels.html:18
>> +    for (var count = 1; count <= 32; ++count) {
> 
> Just curious: Is 32 an arbitrary value? Or do we have Audio elements that use 32 channels?

I suspect it was chosen as a practical limit for the test.

[David Kilzer (:ddkilzer)]

Committed r202617: <http://trac.webkit.org/changeset/202617>

[Eric Carlson]

Comment on attachment 282306 [details]
Patch v1

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

> Source/WebCore/Modules/webaudio/ConvolverNode.cpp:137
> +    bool isChannelCountGood = numberOfChannels == 1 || numberOfChannels == 2 || numberOfChannels == 4;

Is it safe to drop the bufferLength check?

[David Kilzer (:ddkilzer)]

This will be rolled out as the fix wasn't complete.

[Ryan Haddad]

Reverted r202617 for reason:

The LayoutTest from this change crashes under GuardMalloc

Committed r202645: <http://trac.webkit.org/changeset/202645>

[Brent Fulgham]

Created attachment 282848 [details]
Patch

[Brent Fulgham]

Comment on attachment 282848 [details]
Patch

Re-landing after finding the corresponding Blink change to correct the buffer overrun found during our testing.

[Brent Fulgham]

Committed r202845: <http://trac.webkit.org/changeset/202845>