Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1589533002:
Fix fuzzer-found deserialization bugs (Closed)
DescriptionFix fuzzer-found deserialization bugs
This fixes deserialization bugs found by fuzzing SkPaintImageFilter.
BUG=576908, 576910
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1589533002
Committed: https://skia.googlesource.com/skia/+/f8aec588bfd2df17130ee93593a8f4ae781afe1f
Patch Set 1 #Patch Set 2 : Also prevent overflow in SkBufferWithSizeCheck::read #
Total comments: 3
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Remove unreachable code #Patch Set 5 : Fix build error by using explicit cast #
Messages
Total messages: 24 (10 generated)
Description was changed from ========== Fix filter serialization bugs This fixes serialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 ========== to ========== Fix filter serialization bugs This fixes serialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Fix filter serialization bugs This fixes serialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix filter deserialization bugs This fixes deserialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
ajuma@chromium.org changed reviewers: + senorblanco@chromium.org, sugoi@chromium.org
Description was changed from ========== Fix filter deserialization bugs This fixes deserialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix fuzzer-found deserialization bugs This fixes deserialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
senorblanco@chromium.org changed reviewers: + reed@google.com
These look ok to me, but reed@ might want to take a look.
https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1910: fIsVolatile = (packed >> kIsVolatile_SerializationShift) & 0x1; unrelated: this seems wacky -- storing/restoring the volatile bit https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1933: if (buffer.isValid() && pathRef) { I think this if/else-if is getting more confusing. I posit that the following is correct, and perhaps clearer. (I think this should go on line 1912) if (!pathRef) { SkASSERT(!buffer.isValid()); return 0; } Then we shouldn't need to check again.
https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1933: if (buffer.isValid() && pathRef) { On 2016/01/13 19:20:05, reed1 wrote: > I think this if/else-if is getting more confusing. I posit that the following is > correct, and perhaps clearer. > > (I think this should go on line 1912) > > if (!pathRef) { > SkASSERT(!buffer.isValid()); > return 0; > } > > Then we shouldn't need to check again. Moved the check. The suggested SkASSERT doesn't hold though; for example, SkPathRef::CreateFromBuffer can fail because one of the counts read from the buffer is negative (see the changes in SkPathRef.cpp) without the buffer itself becoming invalid.
On 2016/01/13 20:08:15, ajuma wrote: > https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp > File src/core/SkPath.cpp (right): > > https://codereview.chromium.org/1589533002/diff/20001/src/core/SkPath.cpp#new... > src/core/SkPath.cpp:1933: if (buffer.isValid() && pathRef) { > On 2016/01/13 19:20:05, reed1 wrote: > > I think this if/else-if is getting more confusing. I posit that the following > is > > correct, and perhaps clearer. > > > > (I think this should go on line 1912) > > > > if (!pathRef) { > > SkASSERT(!buffer.isValid()); > > return 0; > > } > > > > Then we shouldn't need to check again. > > Moved the check. The suggested SkASSERT doesn't hold though; for example, > SkPathRef::CreateFromBuffer can fail because one of the counts read from the > buffer is negative (see the changes in SkPathRef.cpp) without the buffer itself > becoming invalid. Ah, good point.
https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1917: // If the buffer is not valid, pathRef should be nullptr. why are we throwing, instead of just returning, as we do everywhere else we encounter an invalid buffer?
https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1917: // If the buffer is not valid, pathRef should be nullptr. On 2016/01/13 20:27:11, reed1 wrote: > why are we throwing, instead of just returning, as we do everywhere else we > encounter an invalid buffer? Looking at where this originated (https://codereview.chromium.org/61913002#msg16), I believe the distinction is that this should never be reached, even for an adversarially chosen buffer. The other places where we just return are places that are reachable given particular buffers.
https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1917: // If the buffer is not valid, pathRef should be nullptr. On 2016/01/13 20:47:54, ajuma wrote: > On 2016/01/13 20:27:11, reed1 wrote: > > why are we throwing, instead of just returning, as we do everywhere else we > > encounter an invalid buffer? > > Looking at where this originated > (https://codereview.chromium.org/61913002#msg16), I believe the distinction is > that this should never be reached, even for an adversarially chosen buffer. The > other places where we just return are places that are reachable given particular > buffers. Lets not add code in places that can never be reached. I think this makes the code harder to understand, since it is so different from all other "checks" we do on the buffer. I think we don't need a check at all, and we would still be ok.
https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/1589533002/diff/40001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1917: // If the buffer is not valid, pathRef should be nullptr. On 2016/01/13 21:05:47, reed1 wrote: > On 2016/01/13 20:47:54, ajuma wrote: > > On 2016/01/13 20:27:11, reed1 wrote: > > > why are we throwing, instead of just returning, as we do everywhere else we > > > encounter an invalid buffer? > > > > Looking at where this originated > > (https://codereview.chromium.org/61913002#msg16), I believe the distinction is > > that this should never be reached, even for an adversarially chosen buffer. > The > > other places where we just return are places that are reachable given > particular > > buffers. > > Lets not add code in places that can never be reached. I think this makes the > code harder to understand, since it is so different from all other "checks" we > do on the buffer. > > I think we don't need a check at all, and we would still be ok. Removed the check.
thanks. lgtm
The CQ bit was checked by ajuma@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589533002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589533002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...)
The CQ bit was checked by ajuma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1589533002/#ps80001 (title: "Fix build error by using explicit cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589533002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589533002/80001
Message was sent while issue was closed.
Description was changed from ========== Fix fuzzer-found deserialization bugs This fixes deserialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix fuzzer-found deserialization bugs This fixes deserialization bugs found by fuzzing SkPaintImageFilter. BUG=576908,576910 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f8aec588bfd2df17130ee93593a8f4ae781afe1f ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/f8aec588bfd2df17130ee93593a8f4ae781afe1f |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
