All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
5 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)

Comment hidden (empty)
(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: 779297
(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
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.
Attachment #727594 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Reporter)

Updated

6 years ago
Attachment #727594 - Flags: review?(ehsan) → review+

Comment 6

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

5 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.