Merge cockroachdb#106671

106671: geoindex: error indexing NaN coordinates r=rytaft,sumeerbhola a=otan Resolves cockroachdb#81609 Release note (bug fix): Previously, inserting geometries into a table with an inverted index involving a NaN coordinate could result in a panic. This has now errors instead. Co-authored-by: Oliver Tan <[email protected]>
knz · Jul 17, 2023 · 604d4ae · 604d4ae
2 parents 400e1ad + 677ae02
commit 604d4ae
Show file tree

Hide file tree

Showing 12 changed files with 94 additions and 0 deletions.
diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go
diff --git a/pkg/geo/geoindex/BUILD.bazel b/pkg/geo/geoindex/BUILD.bazel
@@ -21,6 +21,8 @@ go_library(
         "//pkg/geo/geopb",
         "//pkg/geo/geoprojbase",
         "//pkg/geo/geos",
+        "//pkg/sql/pgwire/pgcode",
+        "//pkg/sql/pgwire/pgerror",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_golang_geo//r3",
         "@com_github_golang_geo//s1",

diff --git a/pkg/geo/geoindex/s2_geometry_index.go b/pkg/geo/geoindex/s2_geometry_index.go
@@ -19,6 +19,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/geo/geopb"
 	"github.com/cockroachdb/cockroach/pkg/geo/geoprojbase"
 	"github.com/cockroachdb/cockroach/pkg/geo/geos"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/errors"
 	"github.com/golang/geo/r3"
 	"github.com/golang/geo/s2"
@@ -277,6 +279,14 @@ func (s *s2GeometryIndex) DFullyWithin(
 
 // Converts to geom.T and clips to the rectangle bounds of the index.
 func (s *s2GeometryIndex) convertToGeomTAndTryClip(g geo.Geometry) (geom.T, bool, error) {
+	// Anything with a NaN coordinate should be marked as clipped.
+	if bbox := g.BoundingBoxRef(); bbox != nil && (math.IsNaN(bbox.LoX) || math.IsNaN(bbox.HiX) ||
+		math.IsNaN(bbox.LoY) || math.IsNaN(bbox.HiY)) {
+		return nil, false, pgerror.Newf(
+			pgcode.InvalidParameterValue,
+			"cannot index a geometry with NaN coordinates",
+		)
+	}
 	gt, err := g.AsGeomT()
 	if err != nil {
 		return nil, false, err

diff --git a/pkg/geo/geoindex/testdata/s2_geometry b/pkg/geo/geoindex/testdata/s2_geometry
@@ -16,13 +16,30 @@ geometry name=poly1
 POLYGON((15 15,16 15.5,15.5 17,15 15))
 ----
 
+geometry name=nan
+0105000000060000000102000000020000002CF565DB1790F041010000000000F87F1C1E119D614AE7C1010000000000F87F010200000003000000809745B2972CF841010000000000F87FE0D03302F1C6D641010000000000F87FF83C7FD5C4ACD641010000000000F87F010200000004000000C4A695BE1FC2FD41010000000000F87F42F59F55FD780042010000000000F87FA07574B08DC0D941010000000000F87FD843A641FF49E741010000000000F87F010200000004000000781D60294113F241010000000000F87F100451CE196BED41010000000000F87F1430818C69A2EF41010000000000F87F00E4B02D00CB9341010000000000F87F01020000000400000040B0B6B74C96A0C1010000000000F87F1CD943F294E9E5C1010000000000F87F709F7E29F32AC4C1010000000000F87F607737DC4662E941010000000000F87F010200000004000000F097AF975517F441010000000000F87F5442BA391710F841010000000000F87FE01C9C5F7602B641010000000000F87F40E714D68300E2C1010000000000F87F
+----
+
 geometry name=poly1-different-orientation
 POLYGON((15 15,15.5 17,16 15.5,15 15))
 ----
 
 init minlevel=0 maxlevel=30 maxcells=4 minx=10 miny=10 maxx=20 maxy=20
 ----
 
+# NaN
+covers name=nan
+----
+cannot index a geometry with NaN coordinates
+
+index-keys name=nan
+----
+cannot index a geometry with NaN coordinates
+
+intersects name=nan
+----
+cannot index a geometry with NaN coordinates
+
 # The orientation difference between the polygons has no effect on the
 # index keys.
 

diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial_regression b/pkg/sql/logictest/testdata/logic_test/geospatial_regression
@@ -0,0 +1,9 @@
+statement ok
+CREATE TABLE regression_81609 (
+  g GEOMETRY NOT NULL,
+  INVERTED INDEX (g)
+)
+
+statement error cannot index a geometry with NaN coordinates
+INSERT INTO regression_81609 SELECT st_rotatex('0105000000060000000102000000020000002CF565DB1790F041C2509EC57231F2411C1E119D614AE7C166AFF2D39B030142010200000003000000809745B2972CF84160399714F897E241E0D03302F1C6D6410EFC89BE9FE20042F83C7FD5C4ACD6411854CBFDFF42DE41010200000004000000C4A695BE1FC2FD411CE9BCE66C42D8C142F59F55FD7800424C0D1238165FE041A07574B08DC0D94170D034F5A969EB41D843A641FF49E741B6F0864FF80E0242010200000004000000781D60294113F2412C338441624C02C2100451CE196BED41525C0281389BEEC11430818C69A2EF415C09A4FD752D024200E4B02D00CB9341C047BBFE74DDE44101020000000400000040B0B6B74C96A0C162D23CFAACB8E7C11CD943F294E9E5C187C18248646102C2709F7E29F32AC4C160D1A56EC799FAC1607737DC4662E941ADC9125B944FF6C1010200000004000000F097AF975517F441714AF1717A16F0C15442BA391710F841BEBE416B6780F241E01C9C5F7602B6413CC620A0C9C3E14140E714D68300E2C1A0C4038270B4BAC1':::GEOMETRY::GEOMETRY, '-Inf':::FLOAT8)::GEOMETRY;
+
diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go
diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go
diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go
diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go
diff --git a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go
diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go
diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go