Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268423003:
Adjust gfx::Rect's width and height to avoid integer overflow (Closed)
DescriptionAdjust gfx::Rect's width and height to avoid integer overflow
It is possible that the origin plus the bounds of a gfx::Rect can exceed
the range of an integer, as reflected in clusterfuzz.
This CL makes gfx::Rect adjust the width and height if origin + bounds
can result in an overflow.
BUG=637985
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/86fc67cca4eaad94ec7bf3daebf905939abdd03c
Cr-Commit-Position: refs/heads/master@{#416118}
Patch Set 1 #Patch Set 2 : Fix gfx::Rect::Union instead #
Total comments: 10
Patch Set 3 : Rect ctor validates y + height against integer overflow. #Patch Set 4 : Also fix width #
Total comments: 6
Patch Set 5 : Fix set functions. #Patch Set 6 : Add corresponding test. #Patch Set 7 : Remove repeated test case. #
Total comments: 12
Patch Set 8 : Resolve comments #Patch Set 9 : Make GetClampedValue static. #Patch Set 10 : Try to bypass the compiler's optimization check #Patch Set 11 : Sometimes size could be negative... #
Total comments: 1
Patch Set 12 : Add comment. #
Messages
Total messages: 49 (21 generated)
Description was changed from ========== RenderSurfaceImpl uses SafeUnionRects to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes RenderSurfaceImpl use SafeUnionRects so that the numbers are ascended to int64 types during intermediate calculations. BUG=637985 ========== to ========== RenderSurfaceImpl uses SafeUnionRects to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes RenderSurfaceImpl use SafeUnionRects so that the numbers are ascended to int64 types during intermediate calculations. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
sunxd@chromium.org changed reviewers: + ajuma@chromium.org, danakj@chromium.org, jbroman@chromium.org
Finally I think we should response the clusterfuzz failure with a temp fix. But I'm open to any suggestion that this fix should not be landed and needs to wait for a system fix.
On Tue, Aug 23, 2016 at 10:11 AM, <sunxd@chromium.org> wrote: > Reviewers: danakj, ajuma, jbroman > CL: https://codereview.chromium.org/2268423003/ > > Message: > Finally I think we should response the clusterfuzz failure with a temp > fix. But > I'm open to any suggestion that this fix should not be landed and needs to > wait > for a system fix. > Can you fix gfx::Rect's Union instead? > > Description: > RenderSurfaceImpl uses SafeUnionRects to avoid integer overflow > > It is possible that the origin plus the bounds of a gfx::Rect can exceed > the range of an integer, as reflected in clusterfuzz. > > This CL makes RenderSurfaceImpl use SafeUnionRects so that the numbers > are ascended to int64 types during intermediate calculations. > > BUG=637985 > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+24, -1 lines): > M cc/layers/render_surface_impl.cc > > > Index: cc/layers/render_surface_impl.cc > diff --git a/cc/layers/render_surface_impl.cc b/cc/layers/render_surface_ > impl.cc > index 68c4543dc7071cd201a175787cc5b9bc1e98a7d6.. > bd3a5b361dafaaef3c9a093d6afef4a137b1a37d 100644 > --- a/cc/layers/render_surface_impl.cc > +++ b/cc/layers/render_surface_impl.cc > @@ -28,6 +28,28 @@ > #include "ui/gfx/geometry/rect_conversions.h" > #include "ui/gfx/transform.h" > > +namespace { > +gfx::Rect SafeUnionRects(const gfx::Rect& one, const gfx::Rect& two) { > + if (one.IsEmpty()) > + return two; > + if (two.IsEmpty()) > + return one; > + > + int64_t right1 = static_cast<int64_t>(one.x()) + one.width(); > + int64_t right2 = static_cast<int64_t>(two.x()) + two.width(); > + int64_t bottom1 = static_cast<int64_t>(one.y()) + one.height(); > + int64_t bottom2 = static_cast<int64_t>(two.y()) + two.height(); > + > + int rx = std::min(one.x(), two.x()); > + int ry = std::min(one.y(), two.y()); > + int64_t rr = std::max(right1, right2); > + int64_t rb = std::max(bottom1, bottom2); > + > + return gfx::Rect(rx, ry, static_cast<int>(rr - rx), > + static_cast<int>(rb - ry)); > +} > +} // namespace > + > namespace cc { > > RenderSurfaceImpl::RenderSurfaceImpl(LayerImpl* owning_layer) > @@ -318,7 +340,8 @@ void RenderSurfaceImpl::AccumulateContentRectFromContr > ibutingRenderSurface( > // The content rect of contributing surface is in its own space. Instead, > we > // will use contributing surface's DrawableContentRect which is in target > // space (local space for this render surface) as required. > - accumulated_content_rect_.Union( > + accumulated_content_rect_ = SafeUnionRects( > + accumulated_content_rect_, > gfx::ToEnclosedRect(contributing_surface->DrawableContentRect())); > } > > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/23 17:13:26, danakj wrote: > On Tue, Aug 23, 2016 at 10:11 AM, <mailto:sunxd@chromium.org> wrote: > > > Reviewers: danakj, ajuma, jbroman > > CL: https://codereview.chromium.org/2268423003/ > > > > Message: > > Finally I think we should response the clusterfuzz failure with a temp > > fix. But > > I'm open to any suggestion that this fix should not be landed and needs to > > wait > > for a system fix. > > > > Can you fix gfx::Rect's Union instead? > Do you mean replacing the current logic with the one in this CL? Yes I can do that but I'm not sure if gfx::Rect is supposed to have instances that position.x() + width() > INT_MAX?
On Tue, Aug 23, 2016 at 10:17 AM, <sunxd@chromium.org> wrote: > On 2016/08/23 17:13:26, danakj wrote: > > On Tue, Aug 23, 2016 at 10:11 AM, <mailto:sunxd@chromium.org> wrote: > > > > > Reviewers: danakj, ajuma, jbroman > > > CL: https://codereview.chromium.org/2268423003/ > > > > > > Message: > > > Finally I think we should response the clusterfuzz failure with a temp > > > fix. But > > > I'm open to any suggestion that this fix should not be landed and > needs to > > > wait > > > for a system fix. > > > > > > > Can you fix gfx::Rect's Union instead? > > > > Do you mean replacing the current logic with the one in this CL? Yes I can > do that but I'm not sure if gfx::Rect is supposed to have instances that > position.x() + width() > INT_MAX? > No, because then calling right() will overflow, as demonstrated by this bug. > > https://codereview.chromium.org/2268423003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:148: // integer. This hack should be removed if root cause is fixed. I'm not sure what this comment means, there are like infinity root causes. Rect should simply protect against being invalid. https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:149: int64_t right = static_cast<int64_t>(x()) + width(); why not just use right()? Does the rect already have an invalid right()? Can you fix the place that is causing that in this class too? https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:150: int64_t rect_right = static_cast<int64_t>(rect.x()) + rect.width(); What performs better? int64_ts with (properly) saturating downcast or CheckedNumeric? Why did you choose int64? https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:157: SetRect(rx, ry, static_cast<int>(rr - rx), static_cast<int>(rb - ry)); Does static cast really do what you want? http://stackoverflow.com/questions/5990576/static-cast-with-bounded-types Can you demonstrate that this is doing what you expect with tests, to show that we're not going to get a right/bottom() that overflow?
https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:148: // integer. This hack should be removed if root cause is fixed. On 2016/08/23 17:47:56, danakj wrote: > I'm not sure what this comment means, there are like infinity root causes. Rect > should simply protect against being invalid. I'll remove this comment. https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:149: int64_t right = static_cast<int64_t>(x()) + width(); On 2016/08/23 17:47:56, danakj wrote: > why not just use right()? Does the rect already have an invalid right()? Can you > fix the place that is causing that in this class too? It does not. Is there any difference between right() and bottom() that makes the invalid value come with one but not with the other? https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:150: int64_t rect_right = static_cast<int64_t>(rect.x()) + rect.width(); On 2016/08/23 17:47:56, danakj wrote: > What performs better? int64_ts with (properly) saturating downcast or > CheckedNumeric? Why did you choose int64? Hmm actually I have no idea which has a better performance. I basically just copied the solution in PictureLayerImpl::SafeIntersectRects which was landed to fix a related clusterfuzz bug. I'll switch to CheckedNumeric if that's faster. https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:157: SetRect(rx, ry, static_cast<int>(rr - rx), static_cast<int>(rb - ry)); On 2016/08/23 17:47:56, danakj wrote: > Does static cast really do what you want? > > http://stackoverflow.com/questions/5990576/static-cast-with-bounded-types > > Can you demonstrate that this is doing what you expect with tests, to show that > we're not going to get a right/bottom() that overflow? Working on tests.
https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc File ui/gfx/geometry/rect.cc (right): https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:149: int64_t right = static_cast<int64_t>(x()) + width(); On 2016/08/23 19:08:24, sunxd wrote: > On 2016/08/23 17:47:56, danakj wrote: > > why not just use right()? Does the rect already have an invalid right()? Can > you > > fix the place that is causing that in this class too? > > It does not. Is there any difference between right() and bottom() that makes the > invalid value come with one but not with the other? No, I mean it just depends on the inputs. Neither should be allowed to happen in this class. https://codereview.chromium.org/2268423003/diff/20001/ui/gfx/geometry/rect.cc... ui/gfx/geometry/rect.cc:150: int64_t rect_right = static_cast<int64_t>(rect.x()) + rect.width(); On 2016/08/23 19:08:24, sunxd wrote: > On 2016/08/23 17:47:56, danakj wrote: > > What performs better? int64_ts with (properly) saturating downcast or > > CheckedNumeric? Why did you choose int64? > > Hmm actually I have no idea which has a better performance. I basically just > copied the solution in PictureLayerImpl::SafeIntersectRects which was landed to > fix a related clusterfuzz bug. > > I'll switch to CheckedNumeric if that's faster. You could do a simple test to figure out what's better ala cc_perftests.
The clusterfuzz test case breaks by calling the Rect(x, y, w, h) function with a extremely large 'y'. I think we only need a patch on this function.
On Wed, Aug 24, 2016 at 1:54 PM, <sunxd@chromium.org> wrote: > The clusterfuzz test case breaks by calling the Rect(x, y, w, h) function > with a > extremely large 'y'. I think we only need a patch on this function. > I'd prefer we fix other cases too though, and add tests for them all. Clusterfuzz will continue to find more otherwise. > > https://codereview.chromium.org/2268423003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Aug 24, 2016 at 4:59 PM <danakj@chromium.org> wrote: > On Wed, Aug 24, 2016 at 1:54 PM, <sunxd@chromium.org> wrote: > >> The clusterfuzz test case breaks by calling the Rect(x, y, w, h) function >> with a >> extremely large 'y'. I think we only need a patch on this function. >> > > I'd prefer we fix other cases too though, and add tests for them all. > Clusterfuzz will continue to find more otherwise. > > Would be happy to do that, do we want to apply the same fix for width? ( https://cluster-fuzz.appspot.com/testcase?key=5396030088806400) > >> https://codereview.chromium.org/2268423003/ >> > > -- -Xianda -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Aug 24, 2016 at 2:02 PM, 'Xianda Sun' via cc-bugs < cc-bugs@chromium.org> wrote: > > > On Wed, Aug 24, 2016 at 4:59 PM <danakj@chromium.org> wrote: > >> On Wed, Aug 24, 2016 at 1:54 PM, <sunxd@chromium.org> wrote: >> >>> The clusterfuzz test case breaks by calling the Rect(x, y, w, h) >>> function with a >>> extremely large 'y'. I think we only need a patch on this function. >>> >> >> I'd prefer we fix other cases too though, and add tests for them all. >> Clusterfuzz will continue to find more otherwise. >> >> > Would be happy to do that, do we want to apply the same fix for width? ( > https://cluster-fuzz.appspot.com/testcase?key=5396030088806400) > Yes please, bottom/right should both be valid. > >>> https://codereview.chromium.org/2268423003/ >>> >> >> -- > -Xianda > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
when you publish a new patch you can say "PTAL" or something about what you're publishing for reviewers. just publishing an empty message looks like an accident. :) https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect.h#... ui/gfx/geometry/rect.h:35: #define GetClampedValue(origin, size) \ can this be a constexpr function instead, in an internal namespace? https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect.h#... ui/gfx/geometry/rect.h:74: void set_width(int width) { size_.set_width(width); } How about the non-const methods in this class that can change the size or offset? https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect_un... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect_un... ui/gfx/geometry/rect_unittest.cc:920: EXPECT_EQ(10, height_overflow.height()); can you EXPECT_EQ(max(), height_overflow.bottom()) as well, ditto below? that will help explain it to readers
https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect.h#... ui/gfx/geometry/rect.h:35: #define GetClampedValue(origin, size) \ On 2016/08/25 17:59:21, danakj wrote: > can this be a constexpr function instead, in an internal namespace? Acknowledged. https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect.h#... ui/gfx/geometry/rect.h:74: void set_width(int width) { size_.set_width(width); } On 2016/08/25 17:59:21, danakj wrote: > How about the non-const methods in this class that can change the size or > offset? I'm afraid that clamping values in these functions would hurt the performance. The case where clamping is needed is when we have aggressively huge transforms, which might be a rare case in the real world. But maybe the cost is acceptable? We have both the set functions and operator+=/-= that may modify the components. https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect_un... File ui/gfx/geometry/rect_unittest.cc (right): https://codereview.chromium.org/2268423003/diff/60001/ui/gfx/geometry/rect_un... ui/gfx/geometry/rect_unittest.cc:920: EXPECT_EQ(10, height_overflow.height()); On 2016/08/25 17:59:21, danakj wrote: > can you EXPECT_EQ(max(), height_overflow.bottom()) as well, ditto below? that > will help explain it to readers Acknowledged.
PTAL.
Description was changed from ========== RenderSurfaceImpl uses SafeUnionRects to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes RenderSurfaceImpl use SafeUnionRects so that the numbers are ascended to int64 types during intermediate calculations. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Adjust gfx::Rect's width and height to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes gfx::Rect adjust the width and height if origin + bounds can result in an overflow. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Modify the CL description.
Thanks! A couple more methods and I think it'll be good. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:65: size_.set_width(GetClampedValue(x, size_.width())); nit: you could write this as width() instead of size_.width() https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:71: size_.set_height(GetClampedValue(y, size_.height())); and height() https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:85: set_width(width()); Add a comment // Ensure that width remains valid. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:106: size_.SetSize(width, height); here? https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:118: void Inset(int left, int top, int right, int bottom); Inset calls set_width/height so that's good. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:121: void Offset(int horizontal, int vertical); Offset calls operator+= on origin_ but needs to verify size, like set_origin() https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:123: void operator+=(const Vector2d& offset); ditto for operator+=
PTAL https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:65: size_.set_width(GetClampedValue(x, size_.width())); On 2016/08/29 19:51:01, danakj wrote: > nit: you could write this as width() instead of size_.width() Done. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:71: size_.set_height(GetClampedValue(y, size_.height())); On 2016/08/29 19:51:01, danakj wrote: > and height() Done. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:85: set_width(width()); On 2016/08/29 19:51:01, danakj wrote: > Add a comment // Ensure that width remains valid. Done. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:106: size_.SetSize(width, height); On 2016/08/29 19:51:01, danakj wrote: > here? Done. https://codereview.chromium.org/2268423003/diff/120001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:118: void Inset(int left, int top, int right, int bottom); On 2016/08/29 19:51:01, danakj wrote: > Inset calls set_width/height so that's good. Done.
LGTM
The CQ bit was checked by sunxd@chromium.org
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/08/30 03:30:00, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) > android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > chromeos_amd64-generic_chromium_compile_only_ng on > master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) > chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) > chromeos_x86-generic_chromium_compile_only_ng on > master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) The try bots failed with this message: ../../ui/gfx/geometry/rect.h: In constructor 'constexpr gfx::Rect::Rect(const gfx::Point&, const gfx::Size&)': ../../ui/gfx/geometry/rect.h:46:60: sorry, unimplemented: calling a member function of the object being constructed in a constant expression GetClampedValue(origin.y(), size.height())) {} I think the feature is not supported by the builder on those bots. Should we switch back to macros?
The CQ bit was checked by sunxd@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by sunxd@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunxd@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.
The compiler sometimes knows every value (const) when calling the ctor of gfx::Rect, it tries to optimize the expression and throws a warning. The warning is treated as an error when -Wstrict-overflow is enabled, causing several trybots failure. I tried to fix this by using static_cast<unsigned>.
LGTM https://codereview.chromium.org/2268423003/diff/200001/ui/gfx/geometry/rect.h File ui/gfx/geometry/rect.h (right): https://codereview.chromium.org/2268423003/diff/200001/ui/gfx/geometry/rect.h... ui/gfx/geometry/rect.h:210: return origin > 0 && size > 0 && Can you explain in a comment each of the 3 cases being &&d together here? Something like // 1. If origin <= 0 then size can't be invalid because.. // 2. If size <= 0 then it will be clamped to 0 making x+0 valid for all x. // 3. Using a cast to unsigned because the compiler can optimize this check away entirely but it is not smart enough to know that it won't overflow. It can't overflow since origin is positive as checked by part 1. If size > max - origin then it will overflow when added to origin.
The CQ bit was checked by sunxd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2268423003/#ps220001 (title: "Add comment.")
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 ========== Adjust gfx::Rect's width and height to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes gfx::Rect adjust the width and height if origin + bounds can result in an overflow. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Adjust gfx::Rect's width and height to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes gfx::Rect adjust the width and height if origin + bounds can result in an overflow. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Adjust gfx::Rect's width and height to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes gfx::Rect adjust the width and height if origin + bounds can result in an overflow. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Adjust gfx::Rect's width and height to avoid integer overflow It is possible that the origin plus the bounds of a gfx::Rect can exceed the range of an integer, as reflected in clusterfuzz. This CL makes gfx::Rect adjust the width and height if origin + bounds can result in an overflow. BUG=637985 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/86fc67cca4eaad94ec7bf3daebf905939abdd03c Cr-Commit-Position: refs/heads/master@{#416118} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/86fc67cca4eaad94ec7bf3daebf905939abdd03c Cr-Commit-Position: refs/heads/master@{#416118} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
