Bug 67351 - Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer()
Summary: Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-31 18:29 PDT by Chris Rogers
Modified: 2011-08-31 20:51 PDT (History)
5 users (show)

See Also:

Attachments
Patch (1.64 KB, patch)
2011-08-31 18:30 PDT, Chris Rogers 		no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2011-08-31 18:35 PDT, Chris Rogers 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-08-31 18:29:23 PDT 
Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer()
Comment 1 Chris Rogers 2011-08-31 18:30:50 PDT 
Created attachment 105876 [details]
Patch
Comment 2 Chris Rogers 2011-08-31 18:35:30 PDT 
Created attachment 105877 [details]
Patch
Comment 3 WebKit Review Bot 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>
Comment 4 WebKit Review Bot 2011-08-31 19:07:31 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 5 David Levin 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 < ?
Comment 6 Chris Rogers 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.