Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
718103002:
detect bad bitmaps during deserialization (Closed)
Descriptiondetect bad bitmaps during deserialization
BUG=skia:3117
Committed: https://skia.googlesource.com/skia/+/ac6a2f964ee9821df6a4a8f3c46796322a4c37b8
Patch Set 1 #
Total comments: 1
Patch Set 2 : return true (but empty) if we just didn't recognize the codec type #
Total comments: 8
Messages
Total messages: 21 (8 generated)
reed@google.com changed reviewers: + mtklein@google.com, rmistry@google.com, robertphillips@google.com
I presume this won't "break" other pictures that used to try to continue with empty bitmaps....?
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/718103002/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 2014-11-12 22:17 UTC
The CQ bit was unchecked by reed@google.com
lgtm https://codereview.chromium.org/718103002/diff/1/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/718103002/diff/1/src/core/SkPictureData.cpp#n... src/core/SkPictureData.cpp:398: /* Should we use SkValidatingReadBuffer instead? */ Yeah, I sort of think we should rename SkValidatingReadBuffer SkReadBuffer.
Ravi, can you try applying this patch and see if it indeed fixes the issues you were seeing?
reed@google.com changed reviewers: + scroggo@google.com
The CQ bit was checked by reed@google.com
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/718103002/20001
The CQ bit was unchecked by reed@google.com
On 2014/11/12 16:31:06, reed1 wrote: > Ravi, can you try applying this patch and see if it indeed fixes the issues you > were seeing? Sure, trying it out now.
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/718103002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/ac6a2f964ee9821df6a4a8f3c46796322a4c37b8
Message was sent while issue was closed.
On 2014/11/12 17:12:40, rmistry wrote: > On 2014/11/12 16:31:06, reed1 wrote: > > Ravi, can you try applying this patch and see if it indeed fixes the issues > you > > were seeing? > > Sure, trying it out now. It worked correctly on a single slave (with 8k SKPs). Now that this change is submitted I will trigger a run on the 800k and will check how many times 'failed to load' shows up in the logs.
Message was sent while issue was closed.
https://codereview.chromium.org/718103002/diff/20001/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/718103002/diff/20001/src/core/SkPictureData.c... src/core/SkPictureData.cpp:398: /* Should we use SkValidatingReadBuffer instead? */ If we're going to check isValid, absolutely! Otherwise the checks for isValid are not useful. https://codereview.chromium.org/718103002/diff/20001/src/core/SkPictureData.c... src/core/SkPictureData.cpp:414: if (!buffer.isValid()) { Do we need both checks? Is it possible for isValid to be false when entering the loop (assuming we've changed to SkValidatingReadBuffer)? If not, we can move this check to the end of the while loop and remove the second check. Taken a step further, can buffer.eof() be true when we first enter the loop? I'm guessing only if size is 0? Could we rewrite this loop as: do { tag = buffer.readUInt(); size = buffer.readUInt(); if (!parseBufferTag(buffer, tag, size)) { return false; } if (!buffer.isValid()) { return false; } } while (!buffer.eof()); (As a side note, that still doesn't fix the problem situation where buffer was not at eof at the beginning of the loop, but it is after the first uint, or the second, or part way into parseBufferTag. That seems like a deeper problem, which we may have convinced ourselves we don't need to worry about.) https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cpp File src/core/SkReadBuffer.cpp (right): https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cp... src/core/SkReadBuffer.cpp:262: "Could not decode bitmap. Resulting bitmap will be red."); I think this error message is well out of date. https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cp... src/core/SkReadBuffer.cpp:264: // intact, so we return true with an empty bitmap, so we don't for an abort of the force* ? https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cp... src/core/SkReadBuffer.cpp:273: // just throw this guy away This will fall through and return false. With the change in SkPictureData.cpp, this means we'll abort the picture, right? Does this mean that older SKPs may not work anymore? Is that okay?
Message was sent while issue was closed.
https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cpp File src/core/SkReadBuffer.cpp (right): https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cp... src/core/SkReadBuffer.cpp:262: "Could not decode bitmap. Resulting bitmap will be red."); On 2014/11/12 18:24:48, scroggo wrote: > I think this error message is well out of date. Yea, esp. the RED part. I will update. https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cp... src/core/SkReadBuffer.cpp:264: // intact, so we return true with an empty bitmap, so we don't for an abort of the On 2014/11/12 18:24:48, scroggo wrote: > force* ? Acknowledged. https://codereview.chromium.org/718103002/diff/20001/src/core/SkReadBuffer.cp... src/core/SkReadBuffer.cpp:273: // just throw this guy away On 2014/11/12 18:24:48, scroggo wrote: > This will fall through and return false. With the change in SkPictureData.cpp, > this means we'll abort the picture, right? Does this mean that older SKPs may > not work anymore? Is that okay? This is effectively dead code, which I will clean up later. legacyUnflatten() crashes, so we never actually return from it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
