Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 11 months ago by msarett Modified:
4 years, 11 months ago Reviewers:
scroggo, mtklein CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionRemove size check from SkCodec
This check is useless because it is vulnerable to integer overflow.
Also, I don't think this is the right way to solve the problem
of "too large" images. For example, many image specs allow images
larger than this, so we should too.
BUG=skia:4667
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1580113002
Committed: https://skia.googlesource.com/skia/+/f44631b133909bbf98c13e53ec11bf8cd87a2dc4
Patch Set 1 #Patch Set 2 : Remove from blacklist #
Total comments: 3
Patch Set 3 : Fix return statements #
Created: 4 years, 11 months ago
Depends on Patchset: Messages
Total messages: 13 (7 generated)
Description was changed from ========== Remove size check from SkCodec This check is useless because it is vulernable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. I will follow up with a new approach to this problem. BUG=skia:4667 ========== to ========== Remove size check from SkCodec This check is useless because it is vulernable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. I will follow up with a new approach to this problem. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + mtklein@google.com, scroggo@google.com
> Remove size check from SkCodec > > This check is useless because it is vulernable to integer overflow. nit: vulnerable > Also, I don't think this is the right way to solve the problem > of "too large" images. For example, many image specs allow images > larger than this, so we should too. > > I will follow up with a new approach to this problem. What is the follow up approach? https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:93: return codec.detach(); This seems a little strange until the SkRawCodec code goes in (i.e. we could just return proc.NewFromStream. OTOH, some people prefer a single return statement...)
Description was changed from ========== Remove size check from SkCodec This check is useless because it is vulernable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. I will follow up with a new approach to this problem. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Remove size check from SkCodec This check is useless because it is vulernable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Remove size check from SkCodec This check is useless because it is vulernable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Remove size check from SkCodec This check is useless because it is vulnerable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #3 (id:40001) has been deleted
> > I will follow up with a new approach to this problem. > What is the follow up approach? FWIW, I've removed this line from the commit message. I *think* the follow-up approach might be to not do any size checking (and rely on the client to check). The client is the one allocating memory for the decode anyway. So no follow-up at all :). However, there are still several opened bugs about this, and I don't feel completely sure that we shouldn't do something. https://buganizer.corp.google.com/u/0/issues/26505891 https://bugs.chromium.org/p/skia/issues/detail?id=3617 You could argue that we should wait until these are resolved? But I'd like to go ahead and fix the dumb overflow bug - and I prefer this fix for now. https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:93: return codec.detach(); On 2016/01/12 22:24:20, scroggo wrote: > This seems a little strange until the SkRawCodec code goes in (i.e. we could > just return proc.NewFromStream. OTOH, some people prefer a single return > statement...) You're right. I've fixed this.
lgtm On 2016/01/13 16:38:58, msarett wrote: > > > I will follow up with a new approach to this problem. > > > What is the follow up approach? > > FWIW, I've removed this line from the commit message. > > I *think* the follow-up approach might be to not do any > size checking (and rely on the client to check). The > client is the one allocating memory for the decode anyway. > So no follow-up at all :). > > However, there are still several opened bugs about this, > and I don't feel completely sure that we shouldn't do > something. > https://buganizer.corp.google.com/u/0/issues/26505891 I suspect this is a problem in the RAW code. > https://bugs.chromium.org/p/skia/issues/detail?id=3617 IIUC, this may run out of memory to allocate the output bitmap, but a client can make that decision for themselves when they create the SkCodec and check its dimensions. They could also use scanline decoding/sampling if they don't want the full size. I think it's fine to just remove the check. https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:93: return codec.detach(); On 2016/01/13 16:38:58, msarett wrote: > On 2016/01/12 22:24:20, scroggo wrote: > > This seems a little strange until the SkRawCodec code goes in (i.e. we could > > just return proc.NewFromStream. OTOH, some people prefer a single return > > statement...) > > You're right. I've fixed this. It probably would've been fine to leave it. This change will make them have to undo it, but this is more logical for now. I'm fine either way.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580113002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580113002/60001
Message was sent while issue was closed.
Description was changed from ========== Remove size check from SkCodec This check is useless because it is vulnerable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Remove size check from SkCodec This check is useless because it is vulnerable to integer overflow. Also, I don't think this is the right way to solve the problem of "too large" images. For example, many image specs allow images larger than this, so we should too. BUG=skia:4667 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f44631b133909bbf98c13e53ec11bf8cd87a2dc4 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://skia.googlesource.com/skia/+/f44631b133909bbf98c13e53ec11bf8cd87a2dc4 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
