Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1780643003:
Limit the maximum buffer size of SkRawBufferedStream (Closed)
DescriptionLimit the maximum buffer size of SkRawBufferedStream
BUG=b/27475341
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1780643003
Committed: https://skia.googlesource.com/skia/+/c04df21bb624e299440ce8b56495ec109d4e824e
Patch Set 1 #
Total comments: 8
Patch Set 2 : rename class #Patch Set 3 : rename class #Patch Set 4 : Address comments #Messages
Total messages: 17 (8 generated)
Description was changed from ========== Limit the maximum buffer size of SkRawBufferedStream BUG=skia: ========== to ========== Limit the maximum buffer size of SkRawBufferedStream BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Limit the maximum buffer size of SkRawBufferedStream BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Limit the maximum buffer size of SkRawBufferedStream BUG=b/27475341 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
yujieqin@google.com changed reviewers: + msarett@google.com, scroggo@google.com
The CQ bit was checked by yujieqin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780643003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780643003/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-03-09 22:14 UTC
yujieqin@google.com changed reviewers: + djsollen@google.com
The CQ bit was checked by yujieqin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780643003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780643003/40001
https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:208: if (!safe_add_to_size_t(this->bytesWritten(), size, &newSize) || If we limit to 10,240,000, do we still need to check safe_add_to_size_t? That fits in a 32 bit integer, and I assume size_t is at least that size. I guess we *could* revisit that number, at which point we will need safe_add_to_size_t. If you remove the call, add a comment why it's not necessary. https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:209: newSize > kMaxStreamSize) { nit: this lines up with the line below. Would be better either if it's indented an extra four spaces or if the brace is on the next line by itself. (Unless we remove safe_add_to_size_t, in which case, this fits on one line. https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:210: return false; Should we print some kind of error message? It seems like this *could* be a valid DNG, but we've set a cutoff. https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:212: return SkDynamicMemoryWStream::write(buffer, size); typically in Skia, we use a typedef INHERITED to refer to the base class. So this should have a typedef in private, and this line should look like: return this->INHERITED::write(buffer, size);
PTAL https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:208: if (!safe_add_to_size_t(this->bytesWritten(), size, &newSize) || On 2016/03/09 16:32:45, scroggo wrote: > If we limit to 10,240,000, do we still need to check safe_add_to_size_t? That > fits in a 32 bit integer, and I assume size_t is at least that size. > > I guess we *could* revisit that number, at which point we will need > safe_add_to_size_t. If you remove the call, add a comment why it's not > necessary. I still prefer to check here, because there is not limit on the argument "size", which can surely lead to overflow as soon as the bytesWritten() is larger than 0. https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:209: newSize > kMaxStreamSize) { On 2016/03/09 16:32:45, scroggo wrote: > nit: this lines up with the line below. Would be better either if it's indented > an extra four spaces or if the brace is on the next line by itself. (Unless we > remove safe_add_to_size_t, in which case, this fits on one line. Done. https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:210: return false; On 2016/03/09 16:32:45, scroggo wrote: > Should we print some kind of error message? It seems like this *could* be a > valid DNG, but we've set a cutoff. added a SkCodecPrintf https://codereview.chromium.org/1780643003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:212: return SkDynamicMemoryWStream::write(buffer, size); On 2016/03/09 16:32:45, scroggo wrote: > typically in Skia, we use a typedef INHERITED to refer to the base class. So > this should have a typedef in private, and this line should look like: > > return this->INHERITED::write(buffer, size); Done.
lgtm
The CQ bit was checked by yujieqin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780643003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780643003/60001
Message was sent while issue was closed.
Description was changed from ========== Limit the maximum buffer size of SkRawBufferedStream BUG=b/27475341 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Limit the maximum buffer size of SkRawBufferedStream BUG=b/27475341 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/c04df21bb624e299440ce8b56495ec109d4e824e ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/c04df21bb624e299440ce8b56495ec109d4e824e |
