Closed Bug 853076 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ehsan.akhgari, Assigned: padenot)

References

Details

Attachments

(1 file)

No description provided.
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!
Blocks: webaudio
Summary: Our numFrames calculation code is not safe in safe of large values → Our numFrames calculation code is not safe in face of large values
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.
(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?
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.
Attachment #727594 - Flags: review?(ehsan)
Assignee: nobody → paul
Attachment #727594 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/b06530d4fb32
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 853434
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.