Bug 67351

Summary: Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer()
Product: WebKit Reporter: Chris Rogers <crogers>
Component: New BugsAssignee: Chris Rogers <crogers>
Status: RESOLVED FIXED    
Severity: Normal CC: cevans, dpranke, kbr, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Chris Rogers
Reported 2011-08-31 18:29:23 PDT
Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer()
Attachments
Patch (1.64 KB, patch)
2011-08-31 18:30 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Patch (1.64 KB, patch)
2011-08-31 18:35 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Chris Rogers
Comment 1 2011-08-31 18:30:50 PDT
Created attachment 105876 [details] Patch
Chris Rogers
Comment 2 2011-08-31 18:35:30 PDT
Created attachment 105877 [details] Patch
WebKit Review Bot
Comment 3 2011-08-31 19:07:27 PDT
Comment on attachment 105877 [details] Patch Clearing flags on attachment: 105877 Committed r94265: <http://trac.webkit.org/changeset/94265>
WebKit Review Bot
Comment 4 2011-08-31 19:07:31 PDT
All reviewed patches have been landed. Closing bug.
David Levin
Comment 5 2011-08-31 20:01:20 PDT
It feels like this patch is lacking in the details that would be helpful in the future when someone has to look at this code and try to figure out why it was needed. Here's what seems missing to me. Why is more rigorous bounds checking needed? Is it possible to hit this code or is it some theoretical defensive thing? Why 4096? And why <= as opposed to < ?
Chris Rogers
Comment 6 2011-08-31 20:51:49 PDT
(In reply to comment #5) > It feels like this patch is lacking in the details that would be helpful in the future when someone has to look at this code and try to figure out why it was needed. > > Here's what seems missing to me. > > Why is more rigorous bounds checking needed? > Is it possible to hit this code or is it some theoretical defensive thing? > Why 4096? And why <= as opposed to < ? I can add some more details in comments. In short, this is a defensive check which "probably" should not be possible to hit. The main problem pointed out to me was that there was the potential for integer overflow in the sanity check following this code. This check prevents the overflow from being possible.
Note You need to log in before you can comment on or make changes to this bug.