Fix parsing problems in style file reader

joto · joto · commit 0b3fa706f067 · 2021-12-14T15:10:47.000+01:00
Some variables were not initialized correctly when parsing a style file, which lead to some surprising behaviour with flags of one config line re-used by the next if the flags field of that line was empty. This could also have lead to buffer overflows in the first line being parsed. This commit also adds some more tests for the parsing code. See #1590
diff --git a/src/taginfo.cpp b/src/taginfo.cpp
@@ -84,10 +84,6 @@ static unsigned get_tag_type(std::string const &tag)
 
 bool read_style_file(std::string const &filename, export_list *exlist)
 {
-    char osmtype[24];
-    char tag[64];
-    char datatype[24];
-    char flags[128];
     bool enable_way_area = true;
 
     FILE *const in = std::fopen(filename.c_str(), "rt");
@@ -110,6 +106,10 @@ bool read_style_file(std::string const &filename, export_list *exlist)
         }
 
         //grab the expected fields for this row
+        char osmtype[24] = {0};
+        char tag[64] = {0};
+        char datatype[24] = {0};
+        char flags[128] = {0};
         int const fields = std::sscanf(buffer, "%23s %63s %23s %127s", osmtype,
                                        tag, datatype, flags);
         if (fields <= 0) { /* Blank line */
@@ -124,7 +124,7 @@ bool read_style_file(std::string const &filename, export_list *exlist)
         }
 
         //place to keep info about this tag
-        taginfo temp;
+        taginfo temp{};
         temp.name = tag;
         temp.type = datatype;
         temp.flags = parse_tag_flags(flags, lineno);
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -56,6 +56,7 @@ set_test(test-output-pgsql-area)
 set_test(test-output-pgsql-hstore-match-only)
 set_test(test-output-pgsql-int4)
 set_test(test-output-pgsql-schema)
+set_test(test-output-pgsql-style-file)
 set_test(test-output-pgsql-tablespace LABELS Tablespace)
 set_test(test-output-pgsql-validgeom)
 set_test(test-output-pgsql-z_order)
diff --git a/tests/style/comments.style b/tests/style/comments.style
@@ -0,0 +1,3 @@
+# This is a comment
+
+# Also comment
diff --git a/tests/style/data-types.style b/tests/style/data-types.style
@@ -0,0 +1,4 @@
+# OsmType  Tag         DataType  Flags
+node,way   name        text      linear
+way        width       real      linear
+node,way   population  integer   polygon
diff --git a/tests/style/empty.style b/tests/style/empty.style
diff --git a/tests/style/invalid-data-type.style b/tests/style/invalid-data-type.style
@@ -0,0 +1,2 @@
+# OsmType  Tag         DataType  Flags
+way        highway     foo       linear
diff --git a/tests/style/invalid-osm-type.style b/tests/style/invalid-osm-type.style
@@ -0,0 +1,2 @@
+# OsmType  Tag         DataType  Flags
+foo        bar         text      linear
diff --git a/tests/style/missing.style b/tests/style/missing.style
@@ -0,0 +1,5 @@
+# OsmType  Tag    DataType     Flags
+node       node1  text         linear
+node       node2  text
+way        way1   text         polygon
+way        way2   text
diff --git a/tests/style/node.style b/tests/style/node.style
@@ -0,0 +1 @@
+node access text linear
diff --git a/tests/style/valid.style b/tests/style/valid.style
@@ -0,0 +1,12 @@
+# OsmType  Tag               DataType     Flags
+node       node_text_linear  text         linear
+way        way_text_linear   text         linear
+node,way   both_text_linear  text         linear
+
+node       node_text_polygon text         polygon
+way        way_text_polygon  text         polygon
+node,way   both_text_polygon text         polygon
+
+node,way   both_nocolumn     text         nocolumn
+
+node,way   both_delete       text         delete
diff --git a/tests/style/way-area.style b/tests/style/way-area.style
@@ -0,0 +1,3 @@
+# OsmType  Tag       DataType  Flags
+node,way   wetland   text      polygon,nocolumn
+way        way_area  real
diff --git a/tests/test-output-pgsql-style-file.cpp b/tests/test-output-pgsql-style-file.cpp
@@ -0,0 +1,231 @@
+/**
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This file is part of osm2pgsql (https://osm2pgsql.org/).
+ *
+ * Copyright (C) 2006-2021 by the osm2pgsql developer community.
+ * For a full list of authors see the git log.
+ */
+
+#include <catch.hpp>
+
+#include "common-import.hpp"
+#include "common-options.hpp"
+
+TEST_CASE("Parse default style file")
+{
+    export_list exlist;
+    auto const enable_way_area =
+        read_style_file(OSM2PGSQLDATA_DIR "default.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 98);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 104);
+
+}
+
+TEST_CASE("Parse empty style file")
+{
+    export_list exlist;
+
+    REQUIRE_THROWS_WITH(
+        read_style_file(OSM2PGSQLDATA_DIR "tests/style/empty.style",
+                        &exlist),
+        "Unable to parse any valid columns from the style file. Aborting.");
+}
+
+TEST_CASE("Parse style file with invalid osm type")
+{
+    export_list exlist;
+
+    REQUIRE_THROWS(read_style_file(
+        OSM2PGSQLDATA_DIR "tests/style/invalid-osm-type.style", &exlist));
+}
+
+TEST_CASE("Parse style file with comments only")
+{
+    export_list exlist;
+
+    REQUIRE_THROWS_WITH(
+        read_style_file(OSM2PGSQLDATA_DIR "tests/style/comments.style",
+                        &exlist),
+        "Unable to parse any valid columns from the style file. Aborting.");
+}
+
+TEST_CASE("Parse style file with single node entry")
+{
+    export_list exlist;
+    auto const enable_way_area =
+        read_style_file(OSM2PGSQLDATA_DIR "tests/style/node.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 1);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 0);
+
+    auto const &ex = exlist.get(osmium::item_type::node).front();
+    REQUIRE(ex.name == "access");
+    REQUIRE(ex.type == "text");
+    REQUIRE(ex.flags == column_flags::FLAG_LINEAR);
+    REQUIRE(ex.column_type() == ColumnType::TEXT);
+}
+
+TEST_CASE("Parse style file with a few valid entries")
+{
+    export_list exlist;
+    auto const enable_way_area =
+        read_style_file(OSM2PGSQLDATA_DIR "tests/style/valid.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 6);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 6);
+
+    auto const &nodes = exlist.get(osmium::item_type::node);
+    auto const &ways = exlist.get(osmium::item_type::way);
+
+    for (auto const &node : nodes) {
+        REQUIRE(node.type == "text");
+        REQUIRE(node.column_type() == ColumnType::TEXT);
+    }
+
+    for (auto const &way : ways) {
+        REQUIRE(way.type == "text");
+        REQUIRE(way.column_type() == ColumnType::TEXT);
+    }
+
+    REQUIRE(nodes[0].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(nodes[1].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(nodes[2].flags == column_flags::FLAG_POLYGON);
+    REQUIRE(nodes[3].flags == column_flags::FLAG_POLYGON);
+    REQUIRE(nodes[4].flags == column_flags::FLAG_NOCOLUMN);
+    REQUIRE(nodes[5].flags == column_flags::FLAG_DELETE);
+    REQUIRE(ways[0].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(ways[1].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(ways[2].flags == column_flags::FLAG_POLYGON);
+    REQUIRE(ways[3].flags == column_flags::FLAG_POLYGON);
+    REQUIRE(ways[4].flags == column_flags::FLAG_NOCOLUMN);
+    REQUIRE(ways[5].flags == column_flags::FLAG_DELETE);
+}
+
+TEST_CASE("Parse style file with missing fields")
+{
+    export_list exlist;
+    auto const enable_way_area =
+        read_style_file(OSM2PGSQLDATA_DIR "tests/style/missing.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 2);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 2);
+
+    auto const &nodes = exlist.get(osmium::item_type::node);
+    auto const &ways = exlist.get(osmium::item_type::way);
+
+    for (auto const &node : nodes) {
+        REQUIRE(node.type == "text");
+        REQUIRE(node.column_type() == ColumnType::TEXT);
+    }
+    REQUIRE(nodes[0].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(nodes[1].flags == 0);
+
+    for (auto const &way : ways) {
+        REQUIRE(way.type == "text");
+        REQUIRE(way.column_type() == ColumnType::TEXT);
+    }
+    REQUIRE(ways[0].flags == column_flags::FLAG_POLYGON);
+    REQUIRE(ways[1].flags == 0);
+}
+
+TEST_CASE("Parse style file with way_area")
+{
+    export_list exlist;
+    auto const enable_way_area = read_style_file(
+        OSM2PGSQLDATA_DIR "tests/style/way-area.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 1);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 2);
+
+    auto const &nodes = exlist.get(osmium::item_type::node);
+    auto const &ways = exlist.get(osmium::item_type::way);
+
+    REQUIRE(nodes[0].type == "text");
+    REQUIRE(nodes[0].flags ==
+            (column_flags::FLAG_POLYGON | column_flags::FLAG_NOCOLUMN));
+    REQUIRE(nodes[0].column_type() == ColumnType::TEXT);
+
+    REQUIRE(ways[0].type == "text");
+    REQUIRE(ways[0].flags ==
+            (column_flags::FLAG_POLYGON | column_flags::FLAG_NOCOLUMN));
+    REQUIRE(ways[0].column_type() == ColumnType::TEXT);
+
+    REQUIRE(ways[1].type == "real");
+    REQUIRE(ways[1].flags == 0);
+    REQUIRE(ways[1].column_type() ==
+            ColumnType::TEXT); // Special case for way_area!
+}
+
+TEST_CASE("Parse style file with different data types")
+{
+    export_list exlist;
+    auto const enable_way_area = read_style_file(
+        OSM2PGSQLDATA_DIR "tests/style/data-types.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 2);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 3);
+
+    auto const &nodes = exlist.get(osmium::item_type::node);
+    auto const &ways = exlist.get(osmium::item_type::way);
+
+    REQUIRE(nodes[0].name == "name");
+    REQUIRE(nodes[0].type == "text");
+    REQUIRE(nodes[0].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(nodes[0].column_type() == ColumnType::TEXT);
+
+    REQUIRE(nodes[1].name == "population");
+    REQUIRE(nodes[1].type == "integer");
+    REQUIRE(nodes[1].flags ==
+            (column_flags::FLAG_POLYGON | column_flags::FLAG_INT_TYPE));
+    REQUIRE(nodes[1].column_type() == ColumnType::INT);
+
+    REQUIRE(ways[0].name == "name");
+    REQUIRE(ways[0].type == "text");
+    REQUIRE(ways[0].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(ways[0].column_type() == ColumnType::TEXT);
+
+    REQUIRE(ways[1].name == "width");
+    REQUIRE(ways[1].type == "real");
+    REQUIRE(ways[1].flags ==
+            (column_flags::FLAG_LINEAR | column_flags::FLAG_REAL_TYPE));
+    REQUIRE(ways[1].column_type() == ColumnType::REAL);
+
+    REQUIRE(ways[2].name == "population");
+    REQUIRE(ways[2].type == "integer");
+    REQUIRE(ways[2].flags ==
+            (column_flags::FLAG_POLYGON | column_flags::FLAG_INT_TYPE));
+    REQUIRE(ways[2].column_type() == ColumnType::INT);
+}
+
+TEST_CASE("Parse style file with invalid data types")
+{
+    export_list exlist;
+    auto const enable_way_area = read_style_file(
+        OSM2PGSQLDATA_DIR "tests/style/invalid-data-type.style", &exlist);
+
+    REQUIRE(enable_way_area);
+
+    REQUIRE(exlist.get(osmium::item_type::node).size() == 0);
+    REQUIRE(exlist.get(osmium::item_type::way).size() == 1);
+
+    auto const &ways = exlist.get(osmium::item_type::way);
+
+    REQUIRE(ways[0].name == "highway");
+    REQUIRE(ways[0].type == "foo");
+    REQUIRE(ways[0].flags == column_flags::FLAG_LINEAR);
+    REQUIRE(ways[0].column_type() == ColumnType::TEXT);
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+# This is a comment`
	`2`	`+`
	`3`	`+# Also comment`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+# OsmType Tag DataType Flags`
	`2`	`+way highway foo linear`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+# OsmType Tag DataType Flags`
	`2`	`+foo bar text linear`