Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399043002:
Harden SkPicturePlayback::handleOp() skips (Closed)
DescriptionHarden SkPicturePlayback::handleOp() skips
SkValidatingReadBuffer::skip() may return null - tread more carefully
around it.
BUG=skia:5828
R=reed@google.com,mtklein@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2399043002
Committed: https://skia.googlesource.com/skia/+/d87bd7cfd1dae55fbe4331586aacac3468d59a77
Patch Set 1 #Patch Set 2 : BREAK_ON_READ_ERROR #Messages
Total messages: 19 (12 generated)
Description was changed from ========== Harden SkPicturePlayback::handleOp() skips SkValidatingReadBuffer::skip() may return null - tread more carefully around it. BUG=skia:5828 R=reed@google.com,mtklein@google.com ========== to ========== Harden SkPicturePlayback::handleOp() skips SkValidatingReadBuffer::skip() may return null - tread more carefully around it. BUG=skia:5828 R=reed@google.com,mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2399043002 ==========
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
This seems like it'd be easier to maintain if the null checks are all an
integrated part of each call to skip(). It's not usually too hard to read if
you chain together like this:
... unconditionally safe stuff...
if (auto a = skip(...))
if (auto b = skip(...))
if (auto c = other_thing_that_may_fail()) {
... conditionally safe stuff ...
canvas->call(a,b,c);
}
e.g.
SkCanvas::PointMode mode = reader->readInt();
size_t count = reader->readInt();
if (const SkPoint* pts = (const SkPoint*)reader->skip(sizeof(SkPoint) * count))
if (paint) {
canvas->drawPoints(mode, count, pts, *paint);
}
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/06 16:22:36, mtklein_C wrote:
> This seems like it'd be easier to maintain if the null checks are all an
> integrated part of each call to skip(). It's not usually too hard to read if
> you chain together like this:
>
> ... unconditionally safe stuff...
> if (auto a = skip(...))
> if (auto b = skip(...))
> if (auto c = other_thing_that_may_fail()) {
> ... conditionally safe stuff ...
> canvas->call(a,b,c);
> }
That's pretty cute, if somewhat odd to the untrained eye :)
I just realized there's a more generic approach to validation here: check
reader->isValid() after reading all params, and break (if any of the reads
failed). This also catches all reader errors, not just skips.
Updated PS#2 to try this out, WDYT?
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
break or return -- not sure if one is better (from the macro) lgtm
The CQ bit was checked by mtklein@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Harden SkPicturePlayback::handleOp() skips SkValidatingReadBuffer::skip() may return null - tread more carefully around it. BUG=skia:5828 R=reed@google.com,mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2399043002 ========== to ========== Harden SkPicturePlayback::handleOp() skips SkValidatingReadBuffer::skip() may return null - tread more carefully around it. BUG=skia:5828 R=reed@google.com,mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2399043002 Committed: https://skia.googlesource.com/skia/+/d87bd7cfd1dae55fbe4331586aacac3468d59a77 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/d87bd7cfd1dae55fbe4331586aacac3468d59a77 |
