67351 2011-08-31 18:29:23 -0700 Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer() 2011-08-31 20:51:49 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 crogers crogers cevans dpranke kbr levin webkit.review.bot oldest_to_newest 460139 0 crogers 2011-08-31 18:29:23 -0700 Do more rigorous bounds checking in AudioBufferSourceNode::renderFromBuffer() 460140 1 105876 crogers 2011-08-31 18:30:50 -0700 Created attachment 105876 Patch 460144 2 105877 crogers 2011-08-31 18:35:30 -0700 Created attachment 105877 Patch 460162 3 105877 webkit.review.bot 2011-08-31 19:07:27 -0700 Comment on attachment 105877 Patch Clearing flags on attachment: 105877 Committed r94265: <http://trac.webkit.org/changeset/94265> 460163 4 webkit.review.bot 2011-08-31 19:07:31 -0700 All reviewed patches have been landed. Closing bug. 460182 5 levin 2011-08-31 20:01:20 -0700 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 < ? 460199 6 crogers 2011-08-31 20:51:49 -0700 (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. 105876 2011-08-31 18:30:50 -0700 2011-08-31 18:35:26 -0700 Patch bug-67351-20110831183046.patch text/plain 1679 crogers U3VidmVyc2lvbiBSZXZpc2lvbjogOTQyNTcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA3N2VlZTk4NjQyNTA5MzY2 ZjlhMzc1NmVlYTQ5OWI5OWVjMTVjOWI5Li40NDhmNTUyZjA0YjYzMDQ1N2Q0MjFmZjE3MjUzMDFk NmNmZTJmZGU5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDgtMzEgIENocmlz IFJvZ2VycyAgPGNyb2dlcnNAZ29vZ2xlLmNvbT4KKworICAgICAgICBEbyBtb3JlIHJpZ29yb3Vz IGJvdW5kcyBjaGVja2luZyBpbiBBdWRpb0J1ZmZlclNvdXJjZU5vZGU6OnJlbmRlckZyb21CdWZm ZXIoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njcz NTEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBu ZXcgdGVzdHMgc2luY2UgdGhpcyBkb2VzIG5vdCBjaGFuZ2UgSmF2YVNjcmlwdCBBUEkuCisKKyAg ICAgICAgKiB3ZWJhdWRpby9BdWRpb0J1ZmZlclNvdXJjZU5vZGUuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6QXVkaW9CdWZmZXJTb3VyY2VOb2RlOjpyZW5kZXJGcm9tQnVmZmVyKToKKwogMjAxMS0w OC0zMSAgUHJhdGlrIFNvbGFua2kgIDxwc29sYW5raUBhcHBsZS5jb20+CiAKICAgICAgICAgRml4 IGNvbXBpbGUgaXNzdWVzIHdoZW4gZW5hYmxpbmcgSEFWRShDRk5FVFdPUktfREFUQV9BUlJBWV9D QUxMQkFDSykKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3dlYmF1ZGlvL0F1ZGlvQnVmZmVy U291cmNlTm9kZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS93ZWJhdWRpby9BdWRpb0J1ZmZlclNvdXJj ZU5vZGUuY3BwCmluZGV4IGJmNmM0MjQ4NzNkMDgxNzdlM2RhYmZmMDA5OWFhZWY1NmIxYjIxZjQu LjZkYzIxMmQ3NGYzNjQzNDI4ODAxZmFhNWJmMjEyYWQ3ZDdjZDc4NjYgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3dlYmF1ZGlvL0F1ZGlvQnVmZmVyU291cmNlTm9kZS5jcHAKKysrIGIvU291 cmNlL1dlYkNvcmUvd2ViYXVkaW8vQXVkaW9CdWZmZXJTb3VyY2VOb2RlLmNwcApAQCAtMTg2LDYg KzE4NiwxMiBAQCB2b2lkIEF1ZGlvQnVmZmVyU291cmNlTm9kZTo6cmVuZGVyRnJvbUJ1ZmZlcihB dWRpb0J1cyogYnVzLCB1bnNpZ25lZCBkZXN0aW5hdGlvbgogICAgIAogICAgIC8vIFNhbml0eSBj aGVjayBkZXN0aW5hdGlvbkZyYW1lT2Zmc2V0LCBudW1iZXJPZkZyYW1lcy4KICAgICBzaXplX3Qg ZGVzdGluYXRpb25MZW5ndGggPSBidXMtPmxlbmd0aCgpOworCisgICAgYm9vbCBpc0xlbmd0aEdv b2QgPSBkZXN0aW5hdGlvbkxlbmd0aCA8IDQwOTYgJiYgbnVtYmVyT2ZGcmFtZXMgPCA0MDk2Owor ICAgIEFTU0VSVChpc0xlbmd0aEdvb2QpOworICAgIGlmICghaXNMZW5ndGhHb29kKQorICAgICAg ICByZXR1cm47CisKICAgICBib29sIGlzT2Zmc2V0R29vZCA9IGRlc3RpbmF0aW9uRnJhbWVPZmZz ZXQgPD0gZGVzdGluYXRpb25MZW5ndGggJiYgZGVzdGluYXRpb25GcmFtZU9mZnNldCArIG51bWJl ck9mRnJhbWVzIDw9IGRlc3RpbmF0aW9uTGVuZ3RoOwogICAgIEFTU0VSVChpc09mZnNldEdvb2Qp OwogICAgIGlmICghaXNPZmZzZXRHb29kKQo= 105877 2011-08-31 18:35:30 -0700 2011-08-31 19:07:27 -0700 Patch bug-67351-20110831183526.patch text/plain 1681 crogers U3VidmVyc2lvbiBSZXZpc2lvbjogOTQyNTcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA3N2VlZTk4NjQyNTA5MzY2 ZjlhMzc1NmVlYTQ5OWI5OWVjMTVjOWI5Li40NDhmNTUyZjA0YjYzMDQ1N2Q0MjFmZjE3MjUzMDFk NmNmZTJmZGU5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDgtMzEgIENocmlz IFJvZ2VycyAgPGNyb2dlcnNAZ29vZ2xlLmNvbT4KKworICAgICAgICBEbyBtb3JlIHJpZ29yb3Vz IGJvdW5kcyBjaGVja2luZyBpbiBBdWRpb0J1ZmZlclNvdXJjZU5vZGU6OnJlbmRlckZyb21CdWZm ZXIoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njcz NTEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBu ZXcgdGVzdHMgc2luY2UgdGhpcyBkb2VzIG5vdCBjaGFuZ2UgSmF2YVNjcmlwdCBBUEkuCisKKyAg ICAgICAgKiB3ZWJhdWRpby9BdWRpb0J1ZmZlclNvdXJjZU5vZGUuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6QXVkaW9CdWZmZXJTb3VyY2VOb2RlOjpyZW5kZXJGcm9tQnVmZmVyKToKKwogMjAxMS0w OC0zMSAgUHJhdGlrIFNvbGFua2kgIDxwc29sYW5raUBhcHBsZS5jb20+CiAKICAgICAgICAgRml4 IGNvbXBpbGUgaXNzdWVzIHdoZW4gZW5hYmxpbmcgSEFWRShDRk5FVFdPUktfREFUQV9BUlJBWV9D QUxMQkFDSykKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3dlYmF1ZGlvL0F1ZGlvQnVmZmVy U291cmNlTm9kZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS93ZWJhdWRpby9BdWRpb0J1ZmZlclNvdXJj ZU5vZGUuY3BwCmluZGV4IGJmNmM0MjQ4NzNkMDgxNzdlM2RhYmZmMDA5OWFhZWY1NmIxYjIxZjQu LmRhNzllNjQ4NWE5Mzc0ODdkYWJjY2M4M2QxZTY1YmRkOWNmMGFiZmIgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3dlYmF1ZGlvL0F1ZGlvQnVmZmVyU291cmNlTm9kZS5jcHAKKysrIGIvU291 cmNlL1dlYkNvcmUvd2ViYXVkaW8vQXVkaW9CdWZmZXJTb3VyY2VOb2RlLmNwcApAQCAtMTg2LDYg KzE4NiwxMiBAQCB2b2lkIEF1ZGlvQnVmZmVyU291cmNlTm9kZTo6cmVuZGVyRnJvbUJ1ZmZlcihB dWRpb0J1cyogYnVzLCB1bnNpZ25lZCBkZXN0aW5hdGlvbgogICAgIAogICAgIC8vIFNhbml0eSBj aGVjayBkZXN0aW5hdGlvbkZyYW1lT2Zmc2V0LCBudW1iZXJPZkZyYW1lcy4KICAgICBzaXplX3Qg ZGVzdGluYXRpb25MZW5ndGggPSBidXMtPmxlbmd0aCgpOworCisgICAgYm9vbCBpc0xlbmd0aEdv b2QgPSBkZXN0aW5hdGlvbkxlbmd0aCA8PSA0MDk2ICYmIG51bWJlck9mRnJhbWVzIDw9IDQwOTY7 CisgICAgQVNTRVJUKGlzTGVuZ3RoR29vZCk7CisgICAgaWYgKCFpc0xlbmd0aEdvb2QpCisgICAg ICAgIHJldHVybjsKKwogICAgIGJvb2wgaXNPZmZzZXRHb29kID0gZGVzdGluYXRpb25GcmFtZU9m ZnNldCA8PSBkZXN0aW5hdGlvbkxlbmd0aCAmJiBkZXN0aW5hdGlvbkZyYW1lT2Zmc2V0ICsgbnVt YmVyT2ZGcmFtZXMgPD0gZGVzdGluYXRpb25MZW5ndGg7CiAgICAgQVNTRVJUKGlzT2Zmc2V0R29v ZCk7CiAgICAgaWYgKCFpc09mZnNldEdvb2QpCg==