853076 - Our numFrames calculation code is not safe in face of large values

Status: RESOLVED FIXED

Description

[:Ehsan Akhgari]

Comment 1

[:Ehsan Akhgari]

See this test case:

http://paul.cx/public/decode_testcase.html

On Linux, currentPosition ends up being a very large value for some reason, and then we fall into lala land:

(gdb) l
143	                      uint32_t aBufferOffset,
144	                      uint32_t aBufferMax)
145	  {
146	    uint32_t numFrames = std::min(std::min(WEBAUDIO_BLOCK_SIZE - *aOffsetWithinBlock,
147	                                           aBufferMax - aBufferOffset),
148	                                  uint32_t(mStop - *aCurrentPosition));
149	    if (numFrames == WEBAUDIO_BLOCK_SIZE) {
150	      BorrowFromInputBuffer(aOutput, aChannels, aBufferOffset);
151	    } else {
152	      if (aOutput->IsNull()) {
(gdb) p *aOffsetWithinBlock
$35 = 0
(gdb) p aBufferMax
$36 = 103653
(gdb) p aBufferOffset
$37 = 26022
(gdb) p aBufferMax-aBufferOffset
$38 = 77631
(gdb) p mStop
$39 = 8796093022207
(gdb) p/x mStop
$40 = 0x7ffffffffff
(gdb) p *aCurrentPosition
$41 = 90352942435771968
(gdb) p mStop-*aCurrentPosition
$42 = -90344146342749761
(gdb) p/x *aCurrentPosition
$43 = 0x140ff85be4ba640
(gdb) p (uint32_t)(mStop-*aCurrentPosition)
$44 = 1102338495
(gdb) p numFrames
$45 = 32767

We should make sure that numFrames can never be higher than 128!

Comment 2

[cajbir (:cajbir)]

I've found it helpful to have everywhere we have calculations that might overflow or get too large to PR_STATIC_ASSERT (or similar) that asserts at compile time that it can't happen based upon bounds we've limited the values too.

Comment 3

[:Ehsan Akhgari]

(In reply to comment #2)
> I've found it helpful to have everywhere we have calculations that might
> overflow or get too large to PR_STATIC_ASSERT (or similar) that asserts at
> compile time that it can't happen based upon bounds we've limited the values
> too.

roc, is the position value bounded?

Comment 4

[Paul Adenot (:padenot)]

Created attachment 727594 [details] [diff] [review]
Initialized the chunk to be empty before getting audio frames. r=

I valgrinded a debug build, and I got that: http://www.pastebin.mozilla.org/2233507

Not sure why the AudioChunk are not zeroed when using the default constructor,
but with this patch, everything works just fine.

Comment 5

[:Ehsan Akhgari]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b06530d4fb32

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b06530d4fb32

Comment 7

[:Ehsan Akhgari]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.