Skip to content

Commit 9c47104

Browse files
Ink Open Sourcecopybara-github
authored andcommitted
Fix a CHECK-failure in BrushTipShape constructor for infinitely-large brush tips
Even a valid brush can result in an infinitely-large tip state, due to float overflow, so we shouldn't crash if that happens. However, previously `CalculateCircleRadius` could potentially return NaN for an infinitely-large tip state, causing the `Circle` constructor to CHECK-fail, since it requires a non-negative radius. (This CL also clarifies `Circle`'s API a bit, but doesn't change it.) This crash was found by temporarily re-enabling the stroke generation fuzz test in `strokes/stroke_test.cc`. Fixing this crash is one step toward re-enabling that fuzz test. PiperOrigin-RevId: 827472736
1 parent aadbefc commit 9c47104

File tree

4 files changed

+59
-3
lines changed

4 files changed

+59
-3
lines changed

ink/geometry/internal/circle.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,17 @@ class Circle {
3333
Circle() = default;
3434

3535
// Constructs with the given `center` and `radius`. CHECK-fails if `radius` is
36-
// not greater than or equal to zero.
36+
// negative or NaN.
3737
Circle(Point center, float radius);
3838

3939
Circle(const Circle&) = default;
4040
Circle& operator=(const Circle&) = default;
4141
~Circle() = default;
4242

4343
Point Center() const;
44+
45+
// Returns the radius of the circle. This is guaranteed to be non-negative
46+
// (and not NaN), but may be infinite.
4447
float Radius() const;
4548

4649
// Returns the point on the circle at the given `angle`.

ink/geometry/internal/circle_test.cc

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ using ::testing::FloatNear;
3838
using ::testing::Ge;
3939
using ::testing::SizeIs;
4040

41+
constexpr float kInfinity = std::numeric_limits<float>::infinity();
4142
constexpr float kNan = std::numeric_limits<float>::quiet_NaN();
4243

4344
// Matcher for checking that every `Point` in the `arg` array lies on the
@@ -125,6 +126,12 @@ TEST(CircleTest, ConstructWithZeroRadius) {
125126
EXPECT_EQ(c.Radius(), 0);
126127
}
127128

129+
TEST(CircleTest, ConstructWithInfiniteRadius) {
130+
Circle c({-4, 7}, kInfinity);
131+
EXPECT_THAT(c.Center(), PointEq({-4, 7}));
132+
EXPECT_EQ(c.Radius(), kInfinity);
133+
}
134+
128135
TEST(CircleTest, GetPoint) {
129136
Circle circle({1, 2}, 3.f);
130137

@@ -405,6 +412,10 @@ TEST(CircleDeathTest, ConstructWithNegativeRadius) {
405412
EXPECT_DEATH_IF_SUPPORTED(Circle circle({1, 2}, -1), "");
406413
}
407414

415+
TEST(CircleDeathTest, ConstructWithNanRadius) {
416+
EXPECT_DEATH_IF_SUPPORTED(Circle circle({1, 2}, kNan), "");
417+
}
418+
408419
TEST(CircleDeathTest, AppendArcToPolylineZeroChordHeight) {
409420
Circle circle;
410421
std::vector<Point> polyline;

ink/strokes/internal/brush_tip_shape.cc

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,28 @@ using ::ink::geometry_internal::SegmentIntersection;
5454
float CalculateCircleRadius(float percent_radius, float half_width,
5555
float half_height,
5656
float min_nonzero_radius_and_separation) {
57+
ABSL_DCHECK_GE(percent_radius, 0);
58+
ABSL_DCHECK_LE(percent_radius, 1);
59+
ABSL_DCHECK_GE(half_width, 0);
60+
ABSL_DCHECK_GE(half_height, 0);
61+
ABSL_DCHECK_GE(min_nonzero_radius_and_separation, 0);
62+
63+
// If `percent_radius` is exactly zero, we should always return zero, even if
64+
// `half_width` and `half_height` are infinite (which can happen due to float
65+
// overflow in an earlier calculation). This special case prevents multiplying
66+
// zero times infinity below and getting a radius of NaN (which would then
67+
// CHECK-fail when passed to the `Circle` constructor).
68+
if (percent_radius == 0) {
69+
return 0;
70+
}
71+
5772
float min_half_dimension = std::min(half_width, half_height);
5873
float unmodified_radius = percent_radius * min_half_dimension;
74+
ABSL_DCHECK_GE(unmodified_radius, 0);
5975

60-
// This will result in the shape control circles to be points.
61-
if (unmodified_radius < min_nonzero_radius_and_separation) return 0;
76+
if (unmodified_radius < min_nonzero_radius_and_separation) {
77+
return 0;
78+
}
6279

6380
return unmodified_radius;
6481
}

ink/strokes/internal/brush_tip_shape_test.cc

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
#include "ink/strokes/internal/brush_tip_shape.h"
1616

17+
#include <limits>
18+
1719
#include "gmock/gmock.h"
1820
#include "gtest/gtest.h"
1921
#include "absl/strings/str_cat.h"
@@ -31,9 +33,13 @@ namespace {
3133
using ::ink::geometry_internal::Circle;
3234
using ::ink::geometry_internal::CircleEq;
3335
using ::ink::geometry_internal::CircleNear;
36+
using ::testing::Each;
3437
using ::testing::ElementsAre;
38+
using ::testing::Eq;
39+
using ::testing::Property;
3540

3641
constexpr float kEpsilon = 1e-5;
42+
constexpr float kInfinity = std::numeric_limits<float>::infinity();
3743

3844
BrushTipShape ShapeWithZeroMinRadiusAndSeparation(BrushTipState state) {
3945
return BrushTipShape(state, 0);
@@ -460,6 +466,25 @@ TEST(BrushTipShapeTest, ConstructedWithZeroHeight) {
460466
ElementsAre(CircleEq(Circle({2, 0}, 0)), CircleEq(Circle({-2, 0}, 0))));
461467
}
462468

469+
TEST(BrushTipShapeTest, ConstructedWithInfiniteWidthAndHeightAndZeroRadius) {
470+
// It's possible for a valid brush to end up with an infinitely-large tip
471+
// state, due to float overflow. We should be able to construct a tip shape
472+
// from that without crashing.
473+
BrushTipState state = {
474+
.position = {5, 3},
475+
.width = kInfinity,
476+
.height = kInfinity,
477+
.percent_radius = 0,
478+
};
479+
BrushTipShape shape = ShapeWithZeroMinRadiusAndSeparation(state);
480+
// Even though the tip shape is infinitely large, it should still have a
481+
// well-defined center.
482+
EXPECT_THAT(shape.Center(), PointEq({5, 3}));
483+
// A `percent_radius` of exactly zero should always result in circles of zero
484+
// radius (even though the size is infinite, and zero times infinity is NaN).
485+
EXPECT_THAT(shape.PerimeterCircles(), Each(Property(&Circle::Radius, Eq(0))));
486+
}
487+
463488
TEST(BrushTipShapeDeathTest, ConstructedWithPercentRadiusLessThanZero) {
464489
EXPECT_DEATH_IF_SUPPORTED(BrushTipShape(BrushTipState{.position = {0, 0},
465490
.width = 2,

0 commit comments

Comments
 (0)