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)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: ehsan.akhgari, Assigned: padenot)
References
Details
Attachments
(1 file)
1.05 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•10 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•10 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•10 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•10 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•10 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•10 years ago
|
Assignee: nobody → paul
Reporter | ||
Updated•10 years ago
|
Attachment #727594 -
Flags: review?(ehsan) → review+
Reporter | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06530d4fb32
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b06530d4fb32
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Reporter | ||
Comment 7•9 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.
Description
•