Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
289573002:
Workaround bad bitmaps in clibpoard code (Closed)
|
Created:
6 years, 7 months ago by piman Modified:
6 years, 7 months ago Reviewers:
dcheng CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWorkaround bad bitmaps in clibpoard code
- Some bitmaps end up with a NULL getPixels(). Don't try to copy ot of that.
- Only try to copy 32-bit bitmaps
- Protect against overflow in size computation
BUG=369621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271730
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase #Patch Set 3 : if->DCHECK #
Messages
Total messages: 16 (0 generated)
On 2014/05/14 00:17:36, piman wrote: ping.
On 2014/05/19 19:29:33, piman wrote: > On 2014/05/14 00:17:36, piman wrote: > > ping. Sorry I was in Zurich for BlinkOn last week so I missed this review. Looking now.
https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... File content/renderer/webclipboard_impl.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != SkBitmap::kARGB_8888_Config) Hm... are you aware of any cases where we don't pass in an ARGB bitmap? Otherwise, it seems to me like this should be a CHECK or DCHECK.
https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... File content/renderer/webclipboard_impl.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != SkBitmap::kARGB_8888_Config) On 2014/05/19 19:44:59, dcheng wrote: > Hm... are you aware of any cases where we don't pass in an ARGB bitmap? > Otherwise, it seems to me like this should be a CHECK or DCHECK. Can't it be arbitrary? E.g. what if the png is grayscale?
https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... File content/renderer/webclipboard_impl.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != SkBitmap::kARGB_8888_Config) On 2014/05/19 19:45:56, piman wrote: > On 2014/05/19 19:44:59, dcheng wrote: > > Hm... are you aware of any cases where we don't pass in an ARGB bitmap? > > Otherwise, it seems to me like this should be a CHECK or DCHECK. > > Can't it be arbitrary? > E.g. what if the png is grayscale? I'm not an expert in this area, but this is the decoded bits of the image. I would assume we always decode to a 32-bit ARGB to keep things simple.
https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... File content/renderer/webclipboard_impl.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != SkBitmap::kARGB_8888_Config) On 2014/05/19 19:51:01, dcheng wrote: > On 2014/05/19 19:45:56, piman wrote: > > On 2014/05/19 19:44:59, dcheng wrote: > > > Hm... are you aware of any cases where we don't pass in an ARGB bitmap? > > > Otherwise, it seems to me like this should be a CHECK or DCHECK. > > > > Can't it be arbitrary? > > E.g. what if the png is grayscale? > > I'm not an expert in this area, but this is the decoded bits of the image. I > would assume we always decode to a 32-bit ARGB to keep things simple. I'm not sure we want to make assumptions about code that is far far away in another repo, when that assumption has security implications - see bug. Even if it's true today, is there a test that verifies that it will stay true forever?
On 2014/05/19 22:49:59, piman wrote: > https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... > File content/renderer/webclipboard_impl.cc (right): > > https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... > content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != > SkBitmap::kARGB_8888_Config) > On 2014/05/19 19:51:01, dcheng wrote: > > On 2014/05/19 19:45:56, piman wrote: > > > On 2014/05/19 19:44:59, dcheng wrote: > > > > Hm... are you aware of any cases where we don't pass in an ARGB bitmap? > > > > Otherwise, it seems to me like this should be a CHECK or DCHECK. > > > > > > Can't it be arbitrary? > > > E.g. what if the png is grayscale? > > > > I'm not an expert in this area, but this is the decoded bits of the image. I > > would assume we always decode to a 32-bit ARGB to keep things simple. > > I'm not sure we want to make assumptions about code that is far far away in > another repo, when that assumption has security implications - see bug. Even if > it's true today, is there a test that verifies that it will stay true forever? I'm not saying we should get rid of it--just that it should become a CHECK instead.
On Mon, May 19, 2014 at 3:52 PM, <dcheng@chromium.org> wrote: > On 2014/05/19 22:49:59, piman wrote: > > https://codereview.chromium.org/289573002/diff/1/content/ > renderer/webclipboard_impl.cc > >> File content/renderer/webclipboard_impl.cc (right): >> > > > https://codereview.chromium.org/289573002/diff/1/content/ > renderer/webclipboard_impl.cc#newcode160 > >> content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != >> SkBitmap::kARGB_8888_Config) >> On 2014/05/19 19:51:01, dcheng wrote: >> > On 2014/05/19 19:45:56, piman wrote: >> > > On 2014/05/19 19:44:59, dcheng wrote: >> > > > Hm... are you aware of any cases where we don't pass in an ARGB >> bitmap? >> > > > Otherwise, it seems to me like this should be a CHECK or DCHECK. >> > > >> > > Can't it be arbitrary? >> > > E.g. what if the png is grayscale? >> > >> > I'm not an expert in this area, but this is the decoded bits of the >> image. I >> > would assume we always decode to a 32-bit ARGB to keep things simple. >> > > I'm not sure we want to make assumptions about code that is far far away >> in >> another repo, when that assumption has security implications - see bug. >> Even >> > if > >> it's true today, is there a test that verifies that it will stay true >> forever? >> > > I'm not saying we should get rid of it--just that it should become a CHECK > instead. > We should not have a CHECK in the code base. Either we have a guarantee that the condition is always true, in which case it should be a DCHECK, or we don't in which case an if test (as it is right now) is the correct solution. > > https://codereview.chromium.org/289573002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, May 19, 2014 at 3:57 PM, Antoine Labour <piman@chromium.org> wrote: > > > > On Mon, May 19, 2014 at 3:52 PM, <dcheng@chromium.org> wrote: > >> On 2014/05/19 22:49:59, piman wrote: >> >> https://codereview.chromium.org/289573002/diff/1/content/ >> renderer/webclipboard_impl.cc >> >>> File content/renderer/webclipboard_impl.cc (right): >>> >> >> >> https://codereview.chromium.org/289573002/diff/1/content/ >> renderer/webclipboard_impl.cc#newcode160 >> >>> content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != >>> SkBitmap::kARGB_8888_Config) >>> On 2014/05/19 19:51:01, dcheng wrote: >>> > On 2014/05/19 19:45:56, piman wrote: >>> > > On 2014/05/19 19:44:59, dcheng wrote: >>> > > > Hm... are you aware of any cases where we don't pass in an ARGB >>> bitmap? >>> > > > Otherwise, it seems to me like this should be a CHECK or DCHECK. >>> > > >>> > > Can't it be arbitrary? >>> > > E.g. what if the png is grayscale? >>> > >>> > I'm not an expert in this area, but this is the decoded bits of the >>> image. I >>> > would assume we always decode to a 32-bit ARGB to keep things simple. >>> >> >> I'm not sure we want to make assumptions about code that is far far away >>> in >>> another repo, when that assumption has security implications - see bug. >>> Even >>> >> if >> >>> it's true today, is there a test that verifies that it will stay true >>> forever? >>> >> >> I'm not saying we should get rid of it--just that it should become a CHECK >> instead. >> > > We should not have a CHECK in the code base. > > Either we have a guarantee that the condition is always true, in which > case it should be a DCHECK, or we don't in which case an if test (as it is > right now) is the correct solution. > In particular, the config is explicitly tested in various places in blink (e.g. encoders), which doesn't make me confident that it's ensured to be 32-bits. > >> >> https://codereview.chromium.org/289573002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/19 23:05:59, piman wrote: > On Mon, May 19, 2014 at 3:57 PM, Antoine Labour <mailto:piman@chromium.org> wrote: > > > > > > > > > On Mon, May 19, 2014 at 3:52 PM, <mailto:dcheng@chromium.org> wrote: > > > >> On 2014/05/19 22:49:59, piman wrote: > >> > >> https://codereview.chromium.org/289573002/diff/1/content/ > >> renderer/webclipboard_impl.cc > >> > >>> File content/renderer/webclipboard_impl.cc (right): > >>> > >> > >> > >> https://codereview.chromium.org/289573002/diff/1/content/ > >> renderer/webclipboard_impl.cc#newcode160 > >> > >>> content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != > >>> SkBitmap::kARGB_8888_Config) > >>> On 2014/05/19 19:51:01, dcheng wrote: > >>> > On 2014/05/19 19:45:56, piman wrote: > >>> > > On 2014/05/19 19:44:59, dcheng wrote: > >>> > > > Hm... are you aware of any cases where we don't pass in an ARGB > >>> bitmap? > >>> > > > Otherwise, it seems to me like this should be a CHECK or DCHECK. > >>> > > > >>> > > Can't it be arbitrary? > >>> > > E.g. what if the png is grayscale? > >>> > > >>> > I'm not an expert in this area, but this is the decoded bits of the > >>> image. I > >>> > would assume we always decode to a 32-bit ARGB to keep things simple. > >>> > >> > >> I'm not sure we want to make assumptions about code that is far far away > >>> in > >>> another repo, when that assumption has security implications - see bug. > >>> Even > >>> > >> if > >> > >>> it's true today, is there a test that verifies that it will stay true > >>> forever? > >>> > >> > >> I'm not saying we should get rid of it--just that it should become a CHECK > >> instead. > >> > > > > We should not have a CHECK in the code base. > > > > Either we have a guarantee that the condition is always true, in which > > case it should be a DCHECK, or we don't in which case an if test (as it is > > right now) is the correct solution. > > It shouldn't be an if. There are three non-test implementations of Image. They all return an ARGB32 buffer in this case. BitmapImage: uses ImageDecoder internally. ImageDecoder returns decoded frames as ImageFrame. imageFrame is always an ARGB32 buffer: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... SVGImage: uses ImageBuffer. ImageBuffer always uses an ARGB32 buffer in this case: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... SVGImageForContainer: forwards to SVGImage. > > In particular, the config is explicitly tested in various places in blink > (e.g. encoders), which doesn't make me confident that it's ensured to be > 32-bits. > > > > > >> > >> https://codereview.chromium.org/289573002/ > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. https://codereview.chromium.org/289573002/diff/1/content/renderer/renderer_cl... File content/renderer/renderer_clipboard_client.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/renderer_cl... content/renderer/renderer_clipboard_client.cc:59: uint32 buf_size = checked_buf_size.ValueOrDie(); Maybe we should just pass the SkBitmap to ScopedClipboardWriterGlue? Then we can just do what https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/... does. I'm not really sure which one is simpler though. They're both ugly =/
https://codereview.chromium.org/289573002/diff/1/content/renderer/renderer_cl... File content/renderer/renderer_clipboard_client.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/renderer_cl... content/renderer/renderer_clipboard_client.cc:59: uint32 buf_size = checked_buf_size.ValueOrDie(); On 2014/05/19 23:55:22, dcheng wrote: > Maybe we should just pass the SkBitmap to ScopedClipboardWriterGlue? Then we can > just do what > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/... > does. I'm not really sure which one is simpler though. They're both ugly =/ CheckedNumeric is the security-vetted way of checking for overflow. I'd rather use that. https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... File content/renderer/webclipboard_impl.cc (right): https://codereview.chromium.org/289573002/diff/1/content/renderer/webclipboar... content/renderer/webclipboard_impl.cc:160: if (bitmap.config() != SkBitmap::kARGB_8888_Config) OK, replaced by a DCHECK.
lgtm
The CQ bit was checked by piman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/piman@chromium.org/289573002/40001
Message was sent while issue was closed.
Change committed as 271730 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
