From 87ce5b9310eda00484c47d855d8737f2d802ebc1 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 9 Nov 2017 12:11:07 -0800 Subject: [PATCH 1/4] Be more careful about checking for overflow when parsing numbers --- CHANGELOG.md | 4 ++++ mvt.cpp | 24 +++++++++++++++++++----- tests/overflow/in.json | 3 +++ tests/overflow/out/-z0.json | 22 ++++++++++++++++++++++ version.hpp | 2 +- 5 files changed, 49 insertions(+), 6 deletions(-) create mode 100644 tests/overflow/in.json create mode 100644 tests/overflow/out/-z0.json diff --git a/CHANGELOG.md b/CHANGELOG.md index d646e11..9f118ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 1.26.6 + +* Be more careful about checking for overflow when parsing numbers + ## 1.26.5 * Support UTF-16 surrogate pairs in JSON strings diff --git a/mvt.cpp b/mvt.cpp index 9559dd4..27ee21a 100644 --- a/mvt.cpp +++ b/mvt.cpp @@ -517,14 +517,28 @@ mvt_value stringified_to_mvt_value(int type, const char *s) { tv.numeric_value.sint_value = v; } } else { - double d = atof(s); + errno = 0; + char *endptr; - if (d == (float) d) { - tv.type = mvt_float; - tv.numeric_value.float_value = d; - } else { + float f = strtof(s, &endptr); + + if (endptr == s || ((f == HUGE_VAL || f == HUGE_VALF || f == HUGE_VALL) && errno == ERANGE)) { + double d = strtod(s, &endptr); + if (endptr == s || ((d == HUGE_VAL || d == HUGE_VALF || d == HUGE_VALL) && errno == ERANGE)) { + fprintf(stderr, "Warning: numeric value %s could not be represented\n", s); + } tv.type = mvt_double; tv.numeric_value.double_value = d; + } else { + double d = atof(s); + if (f == d) { + tv.type = mvt_float; + tv.numeric_value.float_value = f; + } else { + // Conversion succeeded, but lost precision, so use double + tv.type = mvt_double; + tv.numeric_value.double_value = d; + } } } } else if (type == mvt_bool) { diff --git a/tests/overflow/in.json b/tests/overflow/in.json new file mode 100644 index 0000000..cbf7d63 --- /dev/null +++ b/tests/overflow/in.json @@ -0,0 +1,3 @@ +{ "type": "Feature", "properties": { "excess": 2222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 22e291 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 2.5 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } diff --git a/tests/overflow/out/-z0.json b/tests/overflow/out/-z0.json new file mode 100644 index 0000000..94df132 --- /dev/null +++ b/tests/overflow/out/-z0.json @@ -0,0 +1,22 @@ +{ "type": "FeatureCollection", "properties": { +"bounds": "0.000000,0.000000,0.000000,0.000000", +"center": "0.000000,0.000000,0", +"description": "tests/overflow/out/-z0.json.check.mbtiles", +"format": "pbf", +"json": "{\"vector_layers\": [ { \"id\": \"in\", \"description\": \"\", \"minzoom\": 0, \"maxzoom\": 0, \"fields\": {\"excess\": \"Number\"} } ],\"tilestats\": {\"layerCount\": 1,\"layers\": [{\"layer\": \"in\",\"count\": 3,\"geometry\": \"Point\",\"attributeCount\": 1,\"attributes\": [{\"attribute\": \"excess\",\"count\": 3,\"type\": \"number\",\"values\": [2.2222222222222223e+291,2.2e+292,2.5],\"min\": 2.5,\"max\": 2.2e+292}]}]}}", +"maxzoom": "0", +"minzoom": "0", +"name": "tests/overflow/out/-z0.json.check.mbtiles", +"type": "overlay", +"version": "2" +}, "features": [ +{ "type": "FeatureCollection", "properties": { "zoom": 0, "x": 0, "y": 0 }, "features": [ +{ "type": "FeatureCollection", "properties": { "layer": "in", "version": 2, "extent": 4096 }, "features": [ +{ "type": "Feature", "properties": { "excess": 2.2222222222222223e+291 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 2.2e+292 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 2.5 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +] } +] } +] } diff --git a/version.hpp b/version.hpp index 0df13f6..096f6e0 100644 --- a/version.hpp +++ b/version.hpp @@ -1,6 +1,6 @@ #ifndef VERSION_HPP #define VERSION_HPP -#define VERSION "tippecanoe v1.26.5\n" +#define VERSION "tippecanoe v1.26.6\n" #endif From aa7191b1eeb2de1f25a7a4730e43a089df7f3beb Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 9 Nov 2017 12:49:09 -0800 Subject: [PATCH 2/4] Also test large integers. Work around an apparent bug in strtoull. --- mvt.cpp | 44 +++++++++++++++++++++++++++++++++++-- mvt.hpp | 3 +++ read_json.cpp | 24 ++++++++++++++++++-- tests/overflow/in.json | 12 ++++++++++ tests/overflow/out/-z0.json | 26 +++++++++++++++++++++- 5 files changed, 104 insertions(+), 5 deletions(-) diff --git a/mvt.cpp b/mvt.cpp index 27ee21a..d1d7452 100644 --- a/mvt.cpp +++ b/mvt.cpp @@ -472,7 +472,7 @@ void mvt_layer::tag(mvt_feature &feature, std::string key, mvt_value value) { feature.tags.push_back(vo); } -static int is_integer(const char *s, long long *v) { +bool is_integer(const char *s, long long *v) { errno = 0; char *endptr; @@ -480,7 +480,47 @@ static int is_integer(const char *s, long long *v) { if (*v == 0 && errno != 0) { return 0; } - if ((*v == LLONG_MIN || *v == LLONG_MAX) && (errno == ERANGE)) { + if ((*v == LLONG_MIN || *v == LLONG_MAX) && (errno == ERANGE || errno == EINVAL)) { + return 0; + } + if (*endptr != '\0') { + // Special case: If it is an integer followed by .0000 or similar, + // it is still an integer + + if (*endptr != '.') { + return 0; + } + endptr++; + for (; *endptr != '\0'; endptr++) { + if (*endptr != '0') { + return 0; + } + } + + return 1; + } + + return 1; +} + +bool is_unsigned_integer(const char *s, unsigned long long *v) { + errno = 0; + char *endptr; + + // Special check because MacOS stroull() returns 1 + // for -18446744073709551615 + while (isspace(*s)) { + s++; + } + if (*s == '-') { + return 0; + } + + *v = strtoull(s, &endptr, 0); + if (*v == 0 && errno != 0) { + return 0; + } + if ((*v == ULLONG_MAX) && (errno == ERANGE || errno == EINVAL)) { return 0; } if (*endptr != '\0') { diff --git a/mvt.hpp b/mvt.hpp index 32ef55f..78539a9 100644 --- a/mvt.hpp +++ b/mvt.hpp @@ -111,4 +111,7 @@ int compress(std::string const &input, std::string &output); int dezig(unsigned n); mvt_value stringified_to_mvt_value(int type, const char *s); + +bool is_integer(const char *s, long long *v); +bool is_unsigned_integer(const char *s, unsigned long long *v); #endif diff --git a/read_json.cpp b/read_json.cpp index bf70e78..5cc63a9 100644 --- a/read_json.cpp +++ b/read_json.cpp @@ -105,7 +105,17 @@ void parse_geometry(int t, json_object *j, drawvec &out, int op, const char *fna void canonicalize(json_object *o) { if (o->type == JSON_NUMBER) { - std::string s = milo::dtoa_milo(o->number); + std::string s; + long long v; + unsigned long long uv; + + if (is_integer(o->string, &v)) { + s = std::to_string(v); + } else if (is_unsigned_integer(o->string, &uv)) { + s = std::to_string(uv); + } else { + s = milo::dtoa_milo(o->number); + } free(o->string); o->string = strdup(s.c_str()); } else if (o->type == JSON_HASH) { @@ -150,7 +160,17 @@ void stringify_value(json_object *value, int &type, std::string &stringified, co } } else if (vt == JSON_NUMBER) { type = mvt_double; - stringified = milo::dtoa_milo(value->number); + + long long v; + unsigned long long uv; + + if (is_integer(value->string, &v)) { + stringified = std::to_string(v); + } else if (is_unsigned_integer(value->string, &uv)) { + stringified = std::to_string(uv); + } else { + stringified = milo::dtoa_milo(value->number); + } } else if (vt == JSON_TRUE || vt == JSON_FALSE) { type = mvt_bool; stringified = val; diff --git a/tests/overflow/in.json b/tests/overflow/in.json index cbf7d63..9aeffd2 100644 --- a/tests/overflow/in.json +++ b/tests/overflow/in.json @@ -1,3 +1,15 @@ { "type": "Feature", "properties": { "excess": 2222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } { "type": "Feature", "properties": { "excess": 22e291 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } { "type": "Feature", "properties": { "excess": 2.5 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 2147483648 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": -2147483648 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 2147483647 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": -2147483647 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 18446744073709551616 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": -18446744073709551616 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 18446744073709551615 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": -18446744073709551615 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 9223372036854775808 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": -9223372036854775808 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": 9223372036854775807 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } +{ "type": "Feature", "properties": { "excess": -9223372036854775807 }, "geometry": { "type": "Point", "coordinates": [ 0,0 ] } } diff --git a/tests/overflow/out/-z0.json b/tests/overflow/out/-z0.json index 94df132..a6f6cdc 100644 --- a/tests/overflow/out/-z0.json +++ b/tests/overflow/out/-z0.json @@ -3,7 +3,7 @@ "center": "0.000000,0.000000,0", "description": "tests/overflow/out/-z0.json.check.mbtiles", "format": "pbf", -"json": "{\"vector_layers\": [ { \"id\": \"in\", \"description\": \"\", \"minzoom\": 0, \"maxzoom\": 0, \"fields\": {\"excess\": \"Number\"} } ],\"tilestats\": {\"layerCount\": 1,\"layers\": [{\"layer\": \"in\",\"count\": 3,\"geometry\": \"Point\",\"attributeCount\": 1,\"attributes\": [{\"attribute\": \"excess\",\"count\": 3,\"type\": \"number\",\"values\": [2.2222222222222223e+291,2.2e+292,2.5],\"min\": 2.5,\"max\": 2.2e+292}]}]}}", +"json": "{\"vector_layers\": [ { \"id\": \"in\", \"description\": \"\", \"minzoom\": 0, \"maxzoom\": 0, \"fields\": {\"excess\": \"Number\"} } ],\"tilestats\": {\"layerCount\": 1,\"layers\": [{\"layer\": \"in\",\"count\": 15,\"geometry\": \"Point\",\"attributeCount\": 1,\"attributes\": [{\"attribute\": \"excess\",\"count\": 14,\"type\": \"number\",\"values\": [-18446744073709553000,-2147483647,-2147483648,-9223372036854775807,-9223372036854775808,18446744073709551615,18446744073709553000,2.2222222222222223e+291,2.2e+292,2.5,2147483647,2147483648,9223372036854775807,9223372036854775808],\"min\": -18446744073709553000,\"max\": 2.2e+292}]}]}}", "maxzoom": "0", "minzoom": "0", "name": "tests/overflow/out/-z0.json.check.mbtiles", @@ -17,6 +17,30 @@ { "type": "Feature", "properties": { "excess": 2.2e+292 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } , { "type": "Feature", "properties": { "excess": 2.5 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 2147483648 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": -2147483648 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 2147483647 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": -2147483647 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": -18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": -18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 9223372036854776000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": -9223372036854775808 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": 9223372036854775807 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "excess": -9223372036854775807 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } ] } ] } ] } From fda0e1f28a6116a5b2a114d2582659b381588e7a Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 9 Nov 2017 13:40:31 -0800 Subject: [PATCH 3/4] Fix more cases of loss of precision for large magnitude integers --- mvt.cpp | 21 +++++++++++++-------- tests/overflow/out/-z0.json | 4 ++-- write_json.cpp | 6 +++--- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/mvt.cpp b/mvt.cpp index d1d7452..c910044 100644 --- a/mvt.cpp +++ b/mvt.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include "mvt.hpp" #include "geometry.hpp" #include "protozero/varint.hpp" @@ -420,7 +421,7 @@ std::string mvt_value::toString() { if (type == mvt_string) { return quote(string_value); } else if (type == mvt_int) { - return std::to_string((long long) numeric_value.int_value); + return std::to_string(numeric_value.int_value); } else if (type == mvt_double) { double v = numeric_value.double_value; if (v == (long long) v) { @@ -436,9 +437,9 @@ std::string mvt_value::toString() { return milo::dtoa_milo(v); } } else if (type == mvt_sint) { - return std::to_string((long long) numeric_value.sint_value); + return std::to_string(numeric_value.sint_value); } else if (type == mvt_uint) { - return std::to_string((long long) numeric_value.uint_value); + return std::to_string(numeric_value.uint_value); } else if (type == mvt_bool) { return numeric_value.bool_value ? "true" : "false"; } else { @@ -548,14 +549,18 @@ mvt_value stringified_to_mvt_value(int type, const char *s) { if (type == mvt_double) { long long v; - if (is_integer(s, &v)) { - if (v >= 0) { + unsigned long long uv; + if (is_unsigned_integer(s, &uv)) { + if (uv <= LLONG_MAX) { tv.type = mvt_int; - tv.numeric_value.int_value = v; + tv.numeric_value.int_value = uv; } else { - tv.type = mvt_sint; - tv.numeric_value.sint_value = v; + tv.type = mvt_uint; + tv.numeric_value.uint_value = uv; } + } else if (is_integer(s, &v)) { + tv.type = mvt_sint; + tv.numeric_value.sint_value = v; } else { errno = 0; char *endptr; diff --git a/tests/overflow/out/-z0.json b/tests/overflow/out/-z0.json index a6f6cdc..0b79124 100644 --- a/tests/overflow/out/-z0.json +++ b/tests/overflow/out/-z0.json @@ -30,11 +30,11 @@ , { "type": "Feature", "properties": { "excess": -18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } , -{ "type": "Feature", "properties": { "excess": 18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +{ "type": "Feature", "properties": { "excess": 18446744073709551615 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } , { "type": "Feature", "properties": { "excess": -18446744073709553000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } , -{ "type": "Feature", "properties": { "excess": 9223372036854776000 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +{ "type": "Feature", "properties": { "excess": 9223372036854775808 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } , { "type": "Feature", "properties": { "excess": -9223372036854775808 }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } , diff --git a/write_json.cpp b/write_json.cpp index f6efb53..ea7a86f 100644 --- a/write_json.cpp +++ b/write_json.cpp @@ -114,7 +114,7 @@ void layer_to_geojson(FILE *fp, mvt_layer const &layer, unsigned z, unsigned x, fprintq(fp, val.string_value.c_str()); } else if (val.type == mvt_int) { fprintq(fp, key); - fprintf(fp, ": %lld", (long long) val.numeric_value.int_value); + fprintf(fp, ": %lld", val.numeric_value.int_value); } else if (val.type == mvt_double) { fprintq(fp, key); double v = val.numeric_value.double_value; @@ -133,10 +133,10 @@ void layer_to_geojson(FILE *fp, mvt_layer const &layer, unsigned z, unsigned x, } } else if (val.type == mvt_sint) { fprintq(fp, key); - fprintf(fp, ": %lld", (long long) val.numeric_value.sint_value); + fprintf(fp, ": %lld", val.numeric_value.sint_value); } else if (val.type == mvt_uint) { fprintq(fp, key); - fprintf(fp, ": %lld", (long long) val.numeric_value.uint_value); + fprintf(fp, ": %llu", val.numeric_value.uint_value); } else if (val.type == mvt_bool) { fprintq(fp, key); fprintf(fp, ": %s", val.numeric_value.bool_value ? "true" : "false"); From 948680fbeb9ec92db25eb1159a04e0d6d2e5ba11 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 9 Nov 2017 14:10:29 -0800 Subject: [PATCH 4/4] Exclude failing overflow test from geobuf tests --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d5c5f11..3969cac 100644 --- a/Makefile +++ b/Makefile @@ -91,7 +91,8 @@ test: tippecanoe tippecanoe-decode $(addsuffix .check,$(TESTS)) raw-tiles-test p cmp $@.out $(patsubst %.check,%,$@) rm $@.out $@.mbtiles -geobuf-test: geojson2nd $(addsuffix .checkbuf,$(TESTS)) +# Don't test overflow with geobuf, because it fails (https://github.com/mapbox/geobuf/issues/87) +geobuf-test: geojson2nd $(addsuffix .checkbuf,$(filter-out tests/overflow/out/-z0.json,$(TESTS))) # For quicker address sanitizer build, hope that regular JSON parsing is tested enough by parallel and join tests fewer-tests: tippecanoe tippecanoe-decode geobuf-test raw-tiles-test parallel-test pbf-test join-test enumerate-test decode-test join-filter-test unit