fix: Crash when metadata contains value with type that is not a list, object, or string (#91)

Uncovered by geoarrow/geoarrow-r#34 !

Basically, because we did a `break` instead of `return EINVAL`, we
exposed uninitialized memory which (sometimes) caused a crash.

I also added the ability to treat edges/crs as `null` as identical to
missing (even though it's technically not valid).

Author: paleolimbot

.gitignore

@@ -21,3 +21,4 @@ out/
 CMakeUserPresets.json
 .vscode
 .Rproj.user
+.cache

src/geoarrow/metadata.c

@@ -109,6 +109,23 @@ static int SkipUntil(struct ArrowStringView* s, const char* items) {
   return 0;
 }
 
+static GeoArrowErrorCode FindNull(struct ArrowStringView* s,
+                                  struct ArrowStringView* out) {
+  if (s->size_bytes < 4) {
+    return EINVAL;
+  }
+
+  if (strncmp(s->data, "null", 4) != 0) {
+    return EINVAL;
+  }
+
+  out->data = s->data;
+  out->size_bytes = 4;
+  s->size_bytes -= 4;
+  s->data += 4;
+  return GEOARROW_OK;
+}
+
 static GeoArrowErrorCode FindString(struct ArrowStringView* s,
                                     struct ArrowStringView* out) {
   out->data = s->data;
@@ -245,25 +262,38 @@ static GeoArrowErrorCode ParseJSONMetadata(struct GeoArrowMetadataView* metadata
       case '\"':
         NANOARROW_RETURN_NOT_OK(FindString(s, &v));
         break;
-      default:
+      case 'n':
+        NANOARROW_RETURN_NOT_OK(FindNull(s, &v));
         break;
+      default:
+        // e.g., a number or boolean
+        return EINVAL;
     }
 
     if (k.size_bytes == 7 && strncmp(k.data, "\"edges\"", 7) == 0) {
       if (v.size_bytes == 11 && strncmp(v.data, "\"spherical\"", 11) == 0) {
         metadata_view->edge_type = GEOARROW_EDGE_TYPE_SPHERICAL;
+      } else if (v.size_bytes == 8 && strncmp(v.data, "\"planar\"", 8) == 0) {
+        metadata_view->edge_type = GEOARROW_EDGE_TYPE_PLANAR;
+      } else if (v.data[0] == 'n') {
+        metadata_view->edge_type = GEOARROW_EDGE_TYPE_PLANAR;
+      } else {
+        return EINVAL;
       }
     } else if (k.size_bytes == 5 && strncmp(k.data, "\"crs\"", 5) == 0) {
       if (v.data[0] == '{') {
         metadata_view->crs_type = GEOARROW_CRS_TYPE_PROJJSON;
+        metadata_view->crs.data = v.data;
+        metadata_view->crs.size_bytes = v.size_bytes;
       } else if (v.data[0] == '\"') {
         metadata_view->crs_type = GEOARROW_CRS_TYPE_UNKNOWN;
+        metadata_view->crs.data = v.data;
+        metadata_view->crs.size_bytes = v.size_bytes;
+      } else if (v.data[0] == 'n') {
+        metadata_view->crs_type = GEOARROW_CRS_TYPE_NONE;
       } else {
         return EINVAL;
       }
-
-      metadata_view->crs.data = v.data;
-      metadata_view->crs.size_bytes = v.size_bytes;
     }
 
     SkipUntil(s, ",}");

src/geoarrow/metadata_test.cc

@@ -98,6 +98,16 @@ TEST(MetadataTest, MetadataTestReadJSONParsing) {
   metadata.data = "{}";
   metadata.size_bytes = 2;
   EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), GEOARROW_OK);
+
+  // Incomplete 'null'
+  metadata.data = "{\"key\": n";
+  metadata.size_bytes = strlen(metadata.data);
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), EINVAL);
+
+  // Enough characters but not actually 'null'
+  metadata.data = "{\"key\": nincompoop}";
+  metadata.size_bytes = strlen(metadata.data);
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), EINVAL);
 }
 
 TEST(MetadataTest, MetadataTestReadJSON) {
@@ -123,6 +133,48 @@ TEST(MetadataTest, MetadataTestReadJSON) {
   EXPECT_EQ(metadata_view.crs_type, GEOARROW_CRS_TYPE_UNKNOWN);
   EXPECT_EQ(std::string(metadata_view.crs.data, metadata_view.crs.size_bytes),
             "\"a string\"");
+
+  const char* json_crs_none = "{\"crs\":null}";
+  metadata.data = json_crs_none;
+  metadata.size_bytes = strlen(json_crs_none);
+
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), GEOARROW_OK);
+  EXPECT_EQ(metadata_view.edge_type, GEOARROW_EDGE_TYPE_PLANAR);
+  EXPECT_EQ(metadata_view.crs_type, GEOARROW_CRS_TYPE_NONE);
+  EXPECT_EQ(metadata_view.crs.size_bytes, 0);
+
+  const char* json_crs_invalid = "{\"crs\":[]}";
+  metadata.data = json_crs_invalid;
+  metadata.size_bytes = strlen(json_crs_invalid);
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), EINVAL);
+
+  const char* json_edges_none = "{\"edges\":null}";
+  metadata.data = json_edges_none;
+  metadata.size_bytes = strlen(json_edges_none);
+
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), GEOARROW_OK);
+  EXPECT_EQ(metadata_view.edge_type, GEOARROW_EDGE_TYPE_PLANAR);
+  EXPECT_EQ(metadata_view.crs_type, GEOARROW_CRS_TYPE_NONE);
+  EXPECT_EQ(metadata_view.crs.size_bytes, 0);
+
+  const char* json_edges_planar = "{\"edges\":\"planar\"}";
+  metadata.data = json_edges_planar;
+  metadata.size_bytes = strlen(json_edges_planar);
+
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), GEOARROW_OK);
+  EXPECT_EQ(metadata_view.edge_type, GEOARROW_EDGE_TYPE_PLANAR);
+  EXPECT_EQ(metadata_view.crs_type, GEOARROW_CRS_TYPE_NONE);
+  EXPECT_EQ(metadata_view.crs.size_bytes, 0);
+
+  const char* json_edges_invalid = "{\"edges\":\"unsupported value\"}";
+  metadata.data = json_edges_invalid;
+  metadata.size_bytes = strlen(json_edges_invalid);
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), EINVAL);
+
+  const char* json_edges_invalid_type = "{\"edges\":true}";
+  metadata.data = json_edges_invalid_type;
+  metadata.size_bytes = strlen(json_edges_invalid_type);
+  EXPECT_EQ(GeoArrowMetadataViewInit(&metadata_view, metadata, &error), EINVAL);
 }
 
 TEST(MetadataTest, MetadataTestSetMetadata) {