Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763613003:
Fix signed integer overflow in ImageData (Closed)
|
Created:
3 years, 9 months ago by zakerinasab Modified:
3 years, 9 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix signed integer overflow in ImageData
Fix signed integer overflow in ImageData catched by ClusterFuzz.
BUG=702972
Review-Url: https://codereview.chromium.org/2763613003
Cr-Commit-Position: refs/heads/master@{#458559}
Committed: https://chromium.googlesource.com/chromium/src/+/e345a1b1fa759b19df872d727b30d34727cc199d
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressing comments #Patch Set 3 : Adding tests #
Total comments: 2
Patch Set 4 : Adding test for too big ImageData #
Total comments: 1
Patch Set 5 : Addressing comments #
Messages
Total messages: 22 (7 generated)
Description was changed from ========== Fix signed integer overflow in ImageData Fix signed integer overflow in ImageData catched by ClusterFuzz. BUG=702972 ========== to ========== Fix signed integer overflow in ImageData Fix signed integer overflow in ImageData catched by ClusterFuzz. BUG=702972 ==========
zakerinasab@chromium.org changed reviewers: + junov@chromium.org
Please add a test. https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/ImageData.cpp (right): https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/ImageData.cpp:184: 4 * (unsigned)(size.width()) * (unsigned)(size.height()), Are we sure that this multiplication will never overlflow the unsigned type?
https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/ImageData.cpp (right): https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/ImageData.cpp:184: 4 * (unsigned)(size.width()) * (unsigned)(size.height()), On 2017/03/20 16:03:55, Justin Novosad wrote: > Are we sure that this multiplication will never overlflow the unsigned type? Yes, this is taken care of in validateConstructorArguments(). Please see line 127: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/Imag...
https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/ImageData.cpp (right): https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/ImageData.cpp:184: 4 * (unsigned)(size.width()) * (unsigned)(size.height()), On 2017/03/20 16:06:33, zakerinasab wrote: > On 2017/03/20 16:03:55, Justin Novosad wrote: > > Are we sure that this multiplication will never overlflow the unsigned type? > > Yes, this is taken care of in validateConstructorArguments As far as I can tell, validateConstructorArguments does not verify that width and height are positive. Also you should not use a c-style cast. Please use static_cast<unsigned>()
https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/ImageData.cpp (right): https://codereview.chromium.org/2763613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/ImageData.cpp:184: 4 * (unsigned)(size.width()) * (unsigned)(size.height()), On 2017/03/20 16:18:40, Justin Novosad wrote: > On 2017/03/20 16:06:33, zakerinasab wrote: > > On 2017/03/20 16:03:55, Justin Novosad wrote: > > > Are we sure that this multiplication will never overlflow the unsigned type? > > > > Yes, this is taken care of in validateConstructorArguments > > As far as I can tell, validateConstructorArguments does not verify that width > and height are positive. Also you should not use a c-style cast. Please use > static_cast<unsigned>() Oh, right. Fixed now.
still needs a test.
On 2017/03/20 17:17:24, Justin Novosad wrote: > still needs a test. Test added.
https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDataTest.cpp:37: imageData = ImageData::create(IntSize(-1, -1)); You forgot to cover large values that overflow into the sign bit. For example the clusterfuzz use case for the bug you are fixing uses (32524, 32587)
On 2017/03/20 18:03:33, Justin Novosad wrote: > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): > > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/ImageDataTest.cpp:37: imageData = > ImageData::create(IntSize(-1, -1)); > You forgot to cover large values that overflow into the sign bit. For example > the clusterfuzz use case for the bug you are fixing uses (32524, 32587) Those large values are supposed to pass the overflow test (as they do not overflow when casted to unsigned). However, they still fail as the requested memory (~4GB) is not allocated. I'm not sure how we should test them.
On 2017/03/20 18:10:31, zakerinasab wrote: > On 2017/03/20 18:03:33, Justin Novosad wrote: > > > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): > > > > > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/ImageDataTest.cpp:37: imageData = > > ImageData::create(IntSize(-1, -1)); > > You forgot to cover large values that overflow into the sign bit. For example > > the clusterfuzz use case for the bug you are fixing uses (32524, 32587) > > Those large values are supposed to pass the overflow test (as they do not > overflow when casted to unsigned). However, they still fail as the requested > memory (~4GB) is not allocated. I'm not sure how we should test them. Inspiration: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/canvas...
https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDataTest.cpp:37: imageData = ImageData::create(IntSize(-1, -1)); On 2017/03/20 18:03:33, Justin Novosad wrote: > You forgot to cover large values that overflow into the sign bit. For example > the clusterfuzz use case for the bug you are fixing uses (32524, 32587) Thanks for the hint. New test added.
zakerinasab@chromium.org changed reviewers: + jochen@chromium.org
On 2017/03/20 20:50:33, zakerinasab wrote: > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): > > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/ImageDataTest.cpp:37: imageData = > ImageData::create(IntSize(-1, -1)); > On 2017/03/20 18:03:33, Justin Novosad wrote: > > You forgot to cover large values that overflow into the sign bit. For example > > the clusterfuzz use case for the bug you are fixing uses (32524, 32587) > > Thanks for the hint. New test added. It seems that the ImageDataTest.CreateImageDataTooBig does not pass on linux_chromium_asan_rel_ng, which means the request to create a very big ImageData is handled successfully. I'll run the trybot again to see if the result is the same or if it is flaky.
On 2017/03/20 20:50:33, zakerinasab wrote: > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): > > https://codereview.chromium.org/2763613003/diff/30002/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/ImageDataTest.cpp:37: imageData = > ImageData::create(IntSize(-1, -1)); > On 2017/03/20 18:03:33, Justin Novosad wrote: > > You forgot to cover large values that overflow into the sign bit. For example > > the clusterfuzz use case for the bug you are fixing uses (32524, 32587) > > Thanks for the hint. New test added. It seems that the ImageDataTest.CreateImageDataTooBig does not pass on linux_chromium_asan_rel_ng, which means the request to create a very big ImageData is handled successfully. I'll run the trybot again to see if the result is the same or if it is flaky.
lgtm with nit. https://codereview.chromium.org/2763613003/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDataTest.cpp (right): https://codereview.chromium.org/2763613003/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDataTest.cpp:44: ImageData* tooBigImageData = ImageData::create(32767, 32767, exceptionState); You need to add a comment here to explain the rationale of this test and what it covers because Chrome is not strictly spec compliant here: The size arguments are 'unsigned long' in the IDL, but they are truncated to int internally, which leads to a very specific overflow risk, which is covered by this test (overflow into the sign bit).
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2763613003/#ps70001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1490126559734220,
"parent_rev": "30574a61c2972095e8dff5a0dc0f7c66e1fcf092", "commit_rev":
"e345a1b1fa759b19df872d727b30d34727cc199d"}
Message was sent while issue was closed.
Description was changed from ========== Fix signed integer overflow in ImageData Fix signed integer overflow in ImageData catched by ClusterFuzz. BUG=702972 ========== to ========== Fix signed integer overflow in ImageData Fix signed integer overflow in ImageData catched by ClusterFuzz. BUG=702972 Review-Url: https://codereview.chromium.org/2763613003 Cr-Commit-Position: refs/heads/master@{#458559} Committed: https://chromium.googlesource.com/chromium/src/+/e345a1b1fa759b19df872d727b30... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001) as https://chromium.googlesource.com/chromium/src/+/e345a1b1fa759b19df872d727b30... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
