Bug 47382 – Blob / BlobBuilder can be put into bad state with wild integers and strings, due to integer overflows

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Evans]

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.

[Chris Evans]

Created attachment 70162 [details]
Patch

[David Levin]

Ideally Jian would review this.

[Eric Seidel (no email)]

Attachment 70162 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4244113

[Chris Evans]

Created attachment 70174 [details]
Patch

[Jian Li]

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.

[Chris Evans]

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.

[Chris Evans]

Created attachment 70178 [details]
Patch

[Jian Li]

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.

[Chris Evans]

Created attachment 70322 [details]
Patch

[Jian Li]

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; ...)

[Jian Li]

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

[Chris Evans]

I did add the Test: line in the latest patch.
I'll fix the "var" thing now and upload a patch.

[Chris Evans]

Created attachment 70654 [details]
Patch

[Jian Li]

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.

[Chris Evans]

Merge done, patch forthcoming.

[Chris Evans]

Created attachment 70661 [details]
Patch

[Jian Li]

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?

[Chris Evans]

Yes. Vector::size() returns size_t, which cannot be stuffed into an "unsigned" on 64-bit without risking truncation.

[WebKit Commit Bot]

Comment on attachment 70661 [details]
Patch

Clearing flags on attachment: 70661

Committed r69716: <http://trac.webkit.org/changeset/69716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/8730623>