|Summary:||MessagePortArray cloning code needs to verify source before copying|
|Product:||WebKit||Reporter:||Andrew Wilson <atwilson>|
|Severity:||Normal||CC:||ademar, commit-queue, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Andrew Wilson 2011-05-19 10:14:13 PDT
The V8 and JSC implementation of MessagePortArray cloning code needs to verify the source array before allocating a copy. For example, the following code will cause an OOM when the bindings code naively tries to allocate a 1234567890 element array: var channel4 = new MessageChannel(); var channel = new MessageChannel(); var largePortArray = ; largePortArray = channel4.port1; channel.port1.postMessage("largeSequence", largePortArray); The correct behavior is to throw an exception because there are undefined elements in the array.
Comment 1 Andrew Wilson 2011-05-19 10:26:31 PDT
Created attachment 94086 [details] Patch to change the MessagePortArray copying code to not pre-allocate the destination. Note that this new code (not pre-allocating the destination) is just as fast as the old code for any arrays containing up to 16 MessagePorts (which covers all expected use cases in practice).
Comment 2 WebKit Review Bot 2011-05-19 10:29:23 PDT
Attachment 94086 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/ChangeLog:10: Line contains tab character. [whitespace/tab]  Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab]  Source/WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab]  Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2011-05-19 11:15:34 PDT
Comment on attachment 94086 [details] Patch to change the MessagePortArray copying code to not pre-allocate the destination. Change looks fine, but tabs in change log will cause the commit queue to fail.
Comment 4 Andrew Wilson 2011-05-19 11:59:05 PDT
Created attachment 94097 [details] Patch without tabs in ChangeLog
Comment 5 Andrew Wilson 2011-05-19 12:55:04 PDT
Comment on attachment 94097 [details] Patch without tabs in ChangeLog I removed those tabs, but the patch is otherwise identical. Anyone want to re-r+ this so the commit-queue can take it?
Comment 6 Darin Adler 2011-05-19 13:28:11 PDT
Comment on attachment 94097 [details] Patch without tabs in ChangeLog Next time I think you should set commit-queue to ? rather than to +. Unless I am ignorant about how commit-queue+ works.
Comment 7 Andrew Wilson 2011-05-19 14:50:00 PDT
You're probably right - it's been a while since I've landed anything but test_expectations changes so I'm a bit rusty on the process :( I should probably also wait on the EWS bots before R? it too. Thanks for the quick turnaround on the reviews!
Comment 8 WebKit Commit Bot 2011-05-19 15:29:36 PDT
Comment on attachment 94097 [details] Patch without tabs in ChangeLog Clearing flags on attachment: 94097 Committed r86899: <http://trac.webkit.org/changeset/86899>
Comment 9 WebKit Commit Bot 2011-05-19 15:29:41 PDT
All reviewed patches have been landed. Closing bug.
Comment 10 Ademar Reis 2011-05-20 05:44:20 PDT
Revision r86899 cherry-picked into qtwebkit-2.2 with commit 63adcb6 <http://gitorious.org/webkit/qtwebkit/commit/63adcb6>