1365225 - Investigate potential integer overflow in dom/media/AudioStream.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Christian Holler (:decoder)]

While looking at static analysis results, I also inspected this:

http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/dom/media/AudioStream.cpp#583-585

I am not sure if it is possible for this to overflow, but if it does, then there is a lot of code behind the putSamples call that I can't verify alone. I inspected some of the memcpy calls afterwards and they all use a product of channel * frames * sizeof(...) which should be fine. Problems would arise as soon as we perform any kind of memory operations on the resulting buffers without the original overflowing multiplication. E.g. if channel * frames * sizeof(...) overflows but channel * frames does not, and we then access or write memory based on that, that would be problematic.

This is all theoretic because I don't know if the multiplication can overflow at all, so that would be the first thing to check.

NI on :jwwang to get started on this :)

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Hi Chun-Min,
Can you take this bug? Thanks!

Comment 2

[C.M.Chang[:chunmin]]

The concern is that: if mOutChannels * c->Frames() is overflowed, then its product will be a smaller number than it should be and we will allocate a smaller space for the buffer. Therefore, part of input data will be put into the invalid address because the the length of data will be larger than the space of buffer.

The types of both `mOutChannels` and `c->Frames()` are uint32_t. The max value of `uint32_t`, is (2^32 - 1), so the max value of their product is (2^32 - 1)^2 = 2^64 - 2 * 2^32 + 1 = (2^64 - 1) - 2^33 + 2 = (max value of uint64_t) - 2^33 + 2 <= (max value of uint64_t). That is, Max(uint32)^2 <= Max(uint64), where Max(T) is the max value of type T.

The type of the product is `AutoTArray<AudioDataValue, *>::size_type`[0] = `size_t`[1]. The size of `size_t` is platform-dependent(it's normally uint64_t nowadays). If the `size_t` is 4 bytes, than the product may be overflowed. If it's 8 bytes, than the current code should be fine. The safest way is to check whether it's overflowed.

About the memory allocating mechanism, I found nothing weird so far. Before calling `putSamples`, the `memset` should work if buf.SetLength[0, 2] return true. Inside `putSamples`[3], the `FIFOSampleBuffer::putSamples`[4] will be called. Before memcpy[5], `FIFOSampleBuffer::ensureCapacity` will be fired to check whether there is enough space. If not, it will throw an error[6].


[0] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/xpcom/ds/nsTArray.h#1836
[1] http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/xpcom/ds/nsTArray.h#394
[2] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/dom/media/AudioStream.cpp#583
[3] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/media/libsoundtouch/src/SoundTouch.cpp#294
[4] http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/media/libsoundtouch/src/FIFOSampleBuffer.cpp#105
[5] http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/media/libsoundtouch/src/FIFOSampleBuffer.cpp#107
[6] http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/media/libsoundtouch/src/FIFOSampleBuffer.cpp#177

Comment 3

[C.M.Chang[:chunmin]]

Attachment: Prevent overflow

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8869331 [details] [diff] [review]
Prevent overflow

Review of attachment 8869331 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/AudioStream.cpp
@@ +579,5 @@
>        mTimeStretcher->putSamples(c->Data(), c->Frames());
>      } else {
>        // Write silence if invalid format.
>        AutoTArray<AudioDataValue, 1000> buf;
> +      CheckedInt<AutoTArray<AudioDataValue, 1000>::size_type> chs(mOutChannels);

You can use CheckedUint32.

Comment 5

[C.M.Chang[:chunmin]]

Attachment: Prevent overflow

Add a log for debugging if there is an overflow happend.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8870285 [details] [diff] [review]
Prevent overflow

Review of attachment 8870285 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/AudioStream.cpp
@@ +590,1 @@
>        memset(buf.Elements(), 0, buf.Length() * sizeof(AudioDataValue));

Also check overflow for |buf.Length() * sizeof(AudioDataValue)|.

Comment 7

[C.M.Chang[:chunmin]]

Attachment: Prevent overflow

Add overflow check for memset.

Comment 8

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f287075098ec1846b4a4453bdb2edbd64b513e0c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7596ff55f5c09779f34d5811d2dcdfb5707990

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e7596ff55f5

Should we consider this for uplift to 54 or let it ride the 55 train?

Comment 11

[C.M.Chang[:chunmin]]

We can uplift if we found any related crash issues. For now, it's only theoretic, so there is no need to do that.