Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1800263002:
Fix pointer aliasing bug in SkImageFilter::computeFastBounds. (Closed)
DescriptionFix pointer aliasing bug in SkImageFilter::computeFastBounds.
Since src and dst are explicitly allowed to alias (according to a comment in
SkPaint.h), it is problematic to have the first input filter mutate dst, since
we still need access to the previous value to provide to the other input
filters.
To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the
stack, and passes that to its inputs instead.
Also add a unit test that would have detected the issue.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1800263002
Committed: https://skia.googlesource.com/skia/+/0e3129d734212089f872f0e7de8f2537861a3f89
Patch Set 1 #
Total comments: 2
Patch Set 2 : copy dst rather than src #
Total comments: 6
Patch Set 3 : code review comments on unit test #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== Fix pointer aliasing bug in SkImageFilter::computeFastBounds. Since src and dst are explicitly allowed to alias (according to a comment in SkPaint.h), it is problematic to have the first input filter mutate dst, since we still need access to the previous value to provide to the other input filters. To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the stack, and passes that to its inputs instead. Also add a unit test that would have detected the issue. ========== to ========== Fix pointer aliasing bug in SkImageFilter::computeFastBounds. Since src and dst are explicitly allowed to alias (according to a comment in SkPaint.h), it is problematic to have the first input filter mutate dst, since we still need access to the previous value to provide to the other input filters. To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the stack, and passes that to its inputs instead. Also add a unit test that would have detected the issue. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800263002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800263002/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-16 01:39 UTC
Description was changed from ========== Fix pointer aliasing bug in SkImageFilter::computeFastBounds. Since src and dst are explicitly allowed to alias (according to a comment in SkPaint.h), it is problematic to have the first input filter mutate dst, since we still need access to the previous value to provide to the other input filters. To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the stack, and passes that to its inputs instead. Also add a unit test that would have detected the issue. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix pointer aliasing bug in SkImageFilter::computeFastBounds. Since src and dst are explicitly allowed to alias (according to a comment in SkPaint.h), it is problematic to have the first input filter mutate dst, since we still need access to the previous value to provide to the other input filters. To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the stack, and passes that to its inputs instead. Also add a unit test that would have detected the issue. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
jbroman@chromium.org changed reviewers: + reed@google.com, senorblanco@chromium.org
Ran into this bug while working in Blink. Fix seems straightforward; PTAL!
https://codereview.chromium.org/1800263002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1800263002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:317: const SkRect srcCopy = src; For clarity, let's just avoid writing to dst until the end of the function (which is what the subclasses do). Something like: SkRect result; if (this->getInput(0)) { this->getInput(0)->computeFastBounds(src, &result); } else { result = src; } } for (int i = 1; i < fInputCount; i++) { SkImageFilter* input = this->getInput(i); if (input) { SkRect bounds; input->computeFastBounds(src, &bounds); result.join(bounds); } else { result.join(src); } *dst = result; }
Let me try that again.
SkRect result;
if (this->getInput(0)) {
this->getInput(0)->computeFastBounds(src, &result);
} else {
result = src;
}
for (int i = 1; i < fInputCount; i++) {
SkImageFilter* input = this->getInput(i);
if (input) {
SkRect bounds;
input->computeFastBounds(src, &bounds);
result.join(bounds);
} else {
result.join(src);
}
}
*dst = result;
https://codereview.chromium.org/1800263002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1800263002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:317: const SkRect srcCopy = src; On 2016/03/15 at 20:39:23, Stephen White wrote: > For clarity, let's just avoid writing to dst until the end of the function (which is what the subclasses do). > > Something like: > > SkRect result; > if (this->getInput(0)) { > this->getInput(0)->computeFastBounds(src, &result); > } else { > result = src; > } > } > > for (int i = 1; i < fInputCount; i++) { > SkImageFilter* input = this->getInput(i); > if (input) { > SkRect bounds; > input->computeFastBounds(src, &bounds); > result.join(bounds); > } else { > result.join(src); > } > *dst = result; > } Done.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800263002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800263002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
reed@google.com changed reviewers: + robertphillips@google.com - reed@google.com
reed@google.com changed reviewers: + reed@google.com
Perhaps we can consider returning the result. That eliminates any confusion/complication around aliasing.
I second Mike's interest in making it structurally more difficult to mess up. That should probably be a separate CL though. https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.c... tests/ImageFilterTest.cpp:756: { add \n somewhere in here to limit to 100 cols ? https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.c... tests/ImageFilterTest.cpp:758: SkRect bounds = SkRect::MakeWH(100, 100); add 'what' after that's ? https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.c... tests/ImageFilterTest.cpp:763: { \n in here too
lgtm
FWIW, I'd have also preferred the signature SkRect(const SkRect&), but that seemed like a separate refactoring if the Skia team wants to undertake it. https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.c... tests/ImageFilterTest.cpp:756: { On 2016/03/16 at 13:54:53, robertphillips wrote: > add \n somewhere in here to limit to 100 cols ? Done. https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.c... tests/ImageFilterTest.cpp:758: SkRect bounds = SkRect::MakeWH(100, 100); On 2016/03/16 at 13:54:53, robertphillips wrote: > add 'what' after that's ? Done. https://codereview.chromium.org/1800263002/diff/20001/tests/ImageFilterTest.c... tests/ImageFilterTest.cpp:763: { On 2016/03/16 at 13:54:53, robertphillips wrote: > \n in here too Done.
LGTM
ping reed for OWNERS?
No needed for owners, but happy to review. lgtm https://bugs.chromium.org/p/skia/issues/detail?id=5094
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1800263002/#ps40001 (title: "code review comments on unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800263002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800263002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix pointer aliasing bug in SkImageFilter::computeFastBounds. Since src and dst are explicitly allowed to alias (according to a comment in SkPaint.h), it is problematic to have the first input filter mutate dst, since we still need access to the previous value to provide to the other input filters. To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the stack, and passes that to its inputs instead. Also add a unit test that would have detected the issue. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix pointer aliasing bug in SkImageFilter::computeFastBounds. Since src and dst are explicitly allowed to alias (according to a comment in SkPaint.h), it is problematic to have the first input filter mutate dst, since we still need access to the previous value to provide to the other input filters. To resolve this, SkImageFilter::computeFastBounds makes a copy of src on the stack, and passes that to its inputs instead. Also add a unit test that would have detected the issue. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/0e3129d734212089f872f0e7de8f2537861a3f89 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/0e3129d734212089f872f0e7de8f2537861a3f89 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
