Bug 47382 - Blob / BlobBuilder can be put into bad state with wild integers and strings, due to integer overflows
Summary: Blob / BlobBuilder can be put into bad state with wild integers and strings, ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chris Evans
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-10-07 15:38 PDT by Chris Evans
Modified: 2010-12-04 16:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2010-10-07 15:56 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Patch (3.73 KB, patch)
2010-10-07 17:08 PDT, Chris Evans
jianli: review-
Details | Formatted Diff | Diff
Patch (3.85 KB, patch)
2010-10-07 17:42 PDT, Chris Evans
jianli: review-
Details | Formatted Diff | Diff
Patch (4.11 KB, patch)
2010-10-08 17:41 PDT, Chris Evans
jianli: review-
Details | Formatted Diff | Diff
Patch (4.12 KB, patch)
2010-10-13 13:44 PDT, Chris Evans
jianli: review-
Details | Formatted Diff | Diff
Patch (4.04 KB, patch)
2010-10-13 14:28 PDT, Chris Evans
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Evans 2010-10-07 15:38:59 PDT
For example:

var bb =  new BlobBuilder();
var s= '';
var i = 0;
for (; i < 2000; ++i) s += 'A';
bb.append(s);
b = bb.getBlob();
slice = b.slice(1999, 9223372036854775000);

This leaves "slice" with a crazy large "size" which is clearly incorrect. I haven't managed to abuse these broken objects to cause any crashing, but I'm going to fix it to be safe.

Patch forthcoming.
Comment 1 Chris Evans 2010-10-07 15:56:45 PDT
Created attachment 70162 [details]
Patch
Comment 2 David Levin 2010-10-07 15:58:19 PDT
Ideally Jian would review this.
Comment 3 Eric Seidel (no email) 2010-10-07 16:05:13 PDT
Attachment 70162 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4244113
Comment 4 Chris Evans 2010-10-07 17:08:10 PDT
Created attachment 70174 [details]
Patch
Comment 5 Jian Li 2010-10-07 17:17:39 PDT
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.
Comment 6 Chris Evans 2010-10-07 17:30:45 PDT
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.
Comment 7 Chris Evans 2010-10-07 17:42:03 PDT
Created attachment 70178 [details]
Patch
Comment 8 Jian Li 2010-10-08 11:23:35 PDT
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.
Comment 9 Chris Evans 2010-10-08 17:41:25 PDT
Created attachment 70322 [details]
Patch
Comment 10 Jian Li 2010-10-11 11:15:35 PDT
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 11 Jian Li 2010-10-11 12:14:43 PDT
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
Comment 12 Chris Evans 2010-10-13 13:41:24 PDT
I did add the Test: line in the latest patch.
I'll fix the "var" thing now and upload a patch.
Comment 13 Chris Evans 2010-10-13 13:44:40 PDT
Created attachment 70654 [details]
Patch
Comment 14 Jian Li 2010-10-13 13:50:10 PDT
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.
Comment 15 Chris Evans 2010-10-13 14:28:32 PDT
Merge done, patch forthcoming.
Comment 16 Chris Evans 2010-10-13 14:28:58 PDT
Created attachment 70661 [details]
Patch
Comment 17 Jian Li 2010-10-13 15:11:46 PDT
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?
Comment 18 Chris Evans 2010-10-13 15:21:14 PDT
Yes. Vector::size() returns size_t, which cannot be stuffed into an "unsigned" on 64-bit without risking truncation.
Comment 19 WebKit Commit Bot 2010-10-13 17:21:51 PDT
Comment on attachment 70661 [details]
Patch

Clearing flags on attachment: 70661

Committed r69716: <http://trac.webkit.org/changeset/69716>
Comment 20 WebKit Commit Bot 2010-10-13 17:21:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 David Kilzer (:ddkilzer) 2010-12-04 16:50:31 PST
<rdar://problem/8730623>