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

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: padenot)

Tracking

Trunk
mozilla22
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

6 years ago
No description provided.
Reporter

Comment 1

6 years ago
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
Reporter

Updated

6 years ago
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

Comment 2

6 years ago
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.
Reporter

Comment 3

6 years ago
(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?
Assignee

Comment 4

6 years ago
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

Updated

6 years ago
Assignee: nobody → paul
Reporter

Updated

6 years ago
Attachment #727594 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/b06530d4fb32
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Updated

6 years ago
Blocks: 853434
Reporter

Comment 7

6 years ago
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.