Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(175)

Issue 2249853008: Reject createImageBitmap promise when the cropRect or resize is too big (Closed)

Created:
4 years, 4 months ago by xidachen
Modified:
4 years, 3 months ago
Reviewers:
Justin Novosad
CC:
chromium-reviews, blink-reviews, posciak+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reject createImageBitmap promise when the cropRect or resize is too big At this moment, creating an ImageBitmap has several options such as flipY and premultiplyAlpha = false. So in some cases, we would have to convert the premultiplied input to unpremul format, and that involves allocating new memory. To prevent any potential integer overflow or OOM situation, this CL checks the size of the cropRect and the resizeWidth(resizeHeight), if the width * height * bytesPerPixel is larger than size_t range, we reject the promise. By doing the check at the beginning of each ImageBitmap constructor, we can guarantee that the subsequent multiplication of width * height * bytesPerPixel will not overflow. This CL also correct other places where there could be potential integer overflow. In particular, since we have checked at the beginning of each ImageBitmap constructor, it should be safe to use size_t for any computation of width * height in the code. TBR=kbr@chromium.org, haraken@chromium.org BUG=638615 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/a43a9eaba800ac7a88b22e8ea6d1666c8dc28ab6 Cr-Commit-Position: refs/heads/master@{#414687}

Patch Set 1 #

Total comments: 11

Patch Set 2 : mostly done #

Patch Set 3 : back to int #

Patch Set 4 : using size_t + partitionAlloc #

Total comments: 2

Patch Set 5 : size_t + Uint8Array + null check (lots) #

Total comments: 1

Patch Set 6 : debugging on win, do not commit #

Total comments: 2

Patch Set 7 : still debugging #

Patch Set 8 : using std::move + leakRef #

Patch Set 9 : debugging #

Total comments: 4

Patch Set 10 : more printf debugging #

Patch Set 11 : fix on win #

Patch Set 12 : update tests #

Messages

Total messages: 31 (15 generated)
xidachen
PTAL
4 years, 4 months ago (2016-08-17 20:38:19 UTC) #3
Justin Novosad
https://codereview.chromium.org/2249853008/diff/1/third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html File third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html (right): https://codereview.chromium.org/2249853008/diff/1/third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html#newcode189 third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html:189: createImageBitmap(element, 0, 0, 0x8000, 0x8000).then(function() { On a 64-bit ...
4 years, 4 months ago (2016-08-18 16:42:14 UTC) #4
xidachen
PTAL. In PS#2, I tried to convert most of int to size_t, but then I ...
4 years, 4 months ago (2016-08-22 12:02:28 UTC) #9
Justin Novosad
On 2016/08/22 12:02:28, xidachen wrote: > PTAL. > > In PS#2, I tried to convert ...
4 years, 4 months ago (2016-08-22 19:17:01 UTC) #10
Justin Novosad
On 2016/08/22 19:17:01, Justin Novosad wrote: > On 2016/08/22 12:02:28, xidachen wrote: > > PTAL. ...
4 years, 4 months ago (2016-08-22 19:17:41 UTC) #11
xidachen
https://codereview.chromium.org/2249853008/diff/60001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2249853008/diff/60001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode463 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:463: void* bufferPtr = partitionAllocGenericFlags(genericAllocator.root(), WTF::PartitionAllocReturnNull, dstBufferSize, "LargeAlloc"); In the ...
4 years, 4 months ago (2016-08-23 01:50:53 UTC) #12
Justin Novosad
https://codereview.chromium.org/2249853008/diff/60001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2249853008/diff/60001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode108 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:108: bool couldDstBufferSizeHasOverflow(ParsedOptions options) Why the "could"?
4 years, 4 months ago (2016-08-23 14:51:08 UTC) #13
xidachen
PS#5 is ready for review. PTAL.
4 years, 4 months ago (2016-08-23 18:51:31 UTC) #16
Justin Novosad
https://codereview.chromium.org/2249853008/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2249853008/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode190 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:190: return newSkImageFromRaster(info, imagePixels, imageRowBytes); should use std::move on imagePixels, ...
4 years, 4 months ago (2016-08-24 13:42:57 UTC) #17
Justin Novosad
https://codereview.chromium.org/2249853008/diff/80001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2249853008/diff/80001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode142 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:142: static PassRefPtr<SkImage> newSkImageFromRaster(const SkImageInfo& info, PassRefPtr<Uint8Array> imagePixels, size_t imageRowBytes) ...
4 years, 4 months ago (2016-08-24 14:49:12 UTC) #18
Justin Novosad
https://codereview.chromium.org/2249853008/diff/160001/third_party/WebKit/Source/core/frame/ImageBitmap.h File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/2249853008/diff/160001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode60 third_party/WebKit/Source/core/frame/ImageBitmap.h:60: PassRefPtr<Uint8Array> copyBitmapData(AlphaDisposition = DontPremultiplyAlpha, DataColorFormat = RGBAColorType); This method ...
4 years, 4 months ago (2016-08-24 14:57:00 UTC) #19
xidachen
The latest patch should pass all tests. PTAL https://codereview.chromium.org/2249853008/diff/160001/third_party/WebKit/Source/core/frame/ImageBitmap.h File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/2249853008/diff/160001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode60 third_party/WebKit/Source/core/frame/ImageBitmap.h:60: PassRefPtr<Uint8Array> ...
4 years, 3 months ago (2016-08-25 20:36:20 UTC) #20
Justin Novosad
lgtm
4 years, 3 months ago (2016-08-25 21:06:29 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2249853008/220001
4 years, 3 months ago (2016-08-26 11:40:37 UTC) #27
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years, 3 months ago (2016-08-26 11:44:20 UTC) #29
commit-bot: I haz the power
4 years, 3 months ago (2016-08-26 11:46:27 UTC) #31
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/a43a9eaba800ac7a88b22e8ea6d1666c8dc28ab6
Cr-Commit-Position: refs/heads/master@{#414687}

Powered by Google App Engine
This is Rietveld 408576698