Summary: | Blob / BlobBuilder can be put into bad state with wild integers and strings, due to integer overflows | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Evans <cevans> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Evans <cevans> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric, jianli, levin | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Attachments: |
|
Description
Chris Evans
2010-10-07 15:38:59 PDT
Created attachment 70162 [details]
Patch
Ideally Jian would review this. Attachment 70162 [details] did not build on mac: Build output: http://queues.webkit.org/results/4244113 Created attachment 70174 [details]
Patch
Comment on attachment 70174 [details] Patch Thanks for finding and fixing the problem. > WebCore/fileapi/BlobBuilder.cpp:69 > + CString result = CString::newUninitialized(newLen, q); Could you please also provide a test case for BlobBuilder.append? > WebCore/ChangeLog:7 > + Fix integer errors in Blob and BlobBuilder. Better to mention the problem in more details, like "Fix integer overflow errors in Blob.slice and BlobBuilder.append". Please update the bug title and ChangeLog to reflect this. > LayoutTests/fast/files/blob-slice-oflow.html:11 > +var bb = new BlobBuilder(); Better to name it as builder. > LayoutTests/fast/files/blob-slice-oflow.html:12 > +var s= ''; Please add a space between 's' and '='. Also, better to rename s as text. > LayoutTests/fast/files/blob-slice-oflow.html:16 > +b = bb.getBlob(); Better to use the meaningful word to name the variable, like "blob". > LayoutTests/fast/files/blob-slice-oflow.html:17 > +slice = b.slice(1999, 9223372036854775000); Better to name it as slicedBlob. > LayoutTests/fast/files/blob-slice-oflow.html:19 > + 'Blob slice length: ' + slice.size)); No need to split into 2 lines. > LayoutTests/fast/files/blob-slice-oflow.html:22 > + alert('FAIL'); Please log the failure instead of popping an alert dialog since otherwise it will cause the test to hang on error. > LayoutTests/ChangeLog:9 > + * fast/files/blob-slice-oflow.html: Added. Would be better not to abbreviate the word. Please rename to blob-slice-overflow.html. Thanks for the review! I will fix all of these things except: "Could you please also provide a test case for BlobBuilder.append?". Unfortunately, any test here would fail on 32-bit (not enough virtual address space) or be inefficient / possibly exhaust physical RAM (64-bit). New patch forthcoming. Created attachment 70178 [details]
Patch
Comment on attachment 70178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70178&action=review > WebCore/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=47382 Normally we prefer to have the lines organized like the following: Bug title Bug link Detailed description Could you please add the bug title before the link? If you add a new test, we should have a line like: Test: fast/files/blob-slice-overflow.html It should be added by prepare-ChangeLog automatically. > LayoutTests/fast/files/blob-slice-overflow.html:14 > +var i = 0; > +for (; i < 2000; ++i) text += 'A'; Can we move "var i = 0" to the for statement since i is not referred outside the for loop? > LayoutTests/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=47382 Please add bug title before the bug link. Created attachment 70322 [details]
Patch
Comment on attachment 70322 [details] Patch This is the only problem. View in context: https://bugs.webkit.org/attachment.cgi?id=70322&action=review > LayoutTests/fast/files/blob-slice-overflow.html:13 > +for (i = 0; i < 2000; ++i) text += 'A'; What I mean is: for (var i = 0; ...) Comment on attachment 70178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70178&action=review > WebCore/ChangeLog:7 > + Fix integer overflow errors in Blob.slice and BlobBuilder.append. One more thing. Please also add an empty line and the following Test line after the description. Test: fast/files/blob-slice-overflow.html I did add the Test: line in the latest patch. I'll fix the "var" thing now and upload a patch. Created attachment 70654 [details]
Patch
Comment on attachment 70654 [details]
Patch
Looks good. But before I approve it, please sync and resolve the conflicts. I am seeing BlobBuilder files have been changed.
Merge done, patch forthcoming. Created attachment 70661 [details]
Patch
Comment on attachment 70661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70661&action=review > WebCore/fileapi/BlobBuilder.cpp:66 > + size_t oldSize = buffer.size(); Is this change now needed? Yes. Vector::size() returns size_t, which cannot be stuffed into an "unsigned" on 64-bit without risking truncation. Comment on attachment 70661 [details] Patch Clearing flags on attachment: 70661 Committed r69716: <http://trac.webkit.org/changeset/69716> All reviewed patches have been landed. Closing bug. |