From e11583df1cec8cac10eb23255835e693031e8894 Mon Sep 17 00:00:00 2001 From: Erica Fischer Date: Fri, 7 Jun 2024 15:01:14 -0700 Subject: [PATCH] Fix hash collision in the string pool (#239) * Current broken behavior * More blatant test * Fix hash collision in string pool * Update version and changelog --- CHANGELOG.md | 4 ++++ pool.cpp | 14 +++++++------- tests/overture-235/in.json | 8 ++++++++ tests/overture-235/out/-z0.json | 26 ++++++++++++++++++++++++++ version.hpp | 2 +- 5 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 tests/overture-235/in.json create mode 100644 tests/overture-235/out/-z0.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 353afaa9..5decd4c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2.55.0 + +* Fix hash collisions in the string pool + # 2.53.0 * Stop trying to add features to the tile after the feature limit is reached diff --git a/pool.cpp b/pool.cpp index 9d4167a5..c23d8ab8 100644 --- a/pool.cpp +++ b/pool.cpp @@ -9,15 +9,15 @@ #include "errors.hpp" #include "text.hpp" -inline int swizzlecmp(const char *a, int atype, unsigned long long ahash, const char *b, int btype, unsigned long long bhash) { - if (ahash == bhash) { +inline long long swizzlecmp(const char *a, int atype, unsigned long long ahash, const char *b, int btype, unsigned long long bhash) { + if ((long long) ahash == (long long) bhash) { if (atype == btype) { return strcmp(a, b); } else { return atype - btype; } } else { - return (int) (ahash - bhash); + return (long long) (ahash - bhash); } } @@ -44,10 +44,10 @@ long long addpool(struct memfile *poolfile, struct memfile *treefile, const char } while (*sp != 0) { - int cmp = swizzlecmp(s, type, hash, - poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off + 1, - (poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off)[0], - ((struct stringpool *) (treefile->map.c_str() + *sp))->hash); + long long cmp = swizzlecmp(s, type, hash, + poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off + 1, + (poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off)[0], + ((struct stringpool *) (treefile->map.c_str() + *sp))->hash); if (cmp < 0) { sp = &(((struct stringpool *) (treefile->map.c_str() + *sp))->left); diff --git a/tests/overture-235/in.json b/tests/overture-235/in.json new file mode 100644 index 00000000..53f0f4e3 --- /dev/null +++ b/tests/overture-235/in.json @@ -0,0 +1,8 @@ +{"type":"Feature","geometry":{"type":"Point","coordinates":[-73.8620223,-15.744153]},"properties":{"id":"0856c5143fffffff0138e27e7a03f7d8","@name":"Andenes","subtype":"locality","local_type":{"en":"hamlet"},"country":"PE","region":"PE-ARE","hierarchies":[[{"division_id":"085bc6b4ffffffff01fb3b67a2808435","subtype":"country","name":"Perú"},{"division_id":"08559acaffffffff01f27abb0a0cb14b","subtype":"region","name":"Arequipa"},{"division_id":"0856c834ffffffff01fd8ac1c98804cb","subtype":"county","name":"Caravelí"},{"division_id":"0856c53b7fffffff019e8d5b8b047485","subtype":"locality","name":"Chaparra"},{"division_id":"0856c5143fffffff0138e27e7a03f7d8","subtype":"locality","name":"Andenes"}]],"parent_division_id":"0856c53b7fffffff019e8d5b8b047485","perspectives":null,"norms":null,"population":null,"capital_division_id":null,"wikidata":null,"names":{"primary":"Andenes","common":{"en":"Andenes","es":"Andenes"},"rules":null},"version":0,"update_time":"2024-05-11T06:56:01Z","sources":[{"property":"","dataset":"OpenStreetMap","record_id":"N3522957503","confidence":null}]},"id":92526} +{"type":"Feature","geometry":{"type":"Point","coordinates":[-0.2216242,51.4948601]},"properties":{"id":"085070173fffffff014fa86038bb5217","@name":"Hammersmith And Fulham","subtype":"county","local_type":{"en":"county"},"country":"GB","region":"GB-ENG","hierarchies":[[{"division_id":"085eea613fffffff01a8a15aa3da4c44","subtype":"country","name":"United Kingdom"},{"division_id":"085255dd7fffffff011aba243af254bd","subtype":"region","name":"England"},{"division_id":"085070173fffffff014fa86038bb5217","subtype":"county","name":"Hammersmith And Fulham"}]],"parent_division_id":"085255dd7fffffff011aba243af254bd","perspectives":null,"norms":null,"population":null,"capital_division_id":null,"wikidata":null,"names":{"primary":"Hammersmith And Fulham","common":null,"rules":null},"version":0,"update_time":"2024-05-11T06:57:45Z","sources":[{"property":"","dataset":"geoBoundaries","record_id":"GBR9080712B6991151074246","confidence":null}]},"id":1096239} + + +{"type": "Feature", "properties": {"one": "Poull Bojer"}, "geometry": {"type":"Point","coordinates":[0,0]}} +{"type": "Feature", "properties": {"two": +"[[{\"division_id\":\"085ec5c23fffffff01339442745f3d55\",\"subtype\":\"country\",\"name\":\"Ecuador\"},{\"division_id\":\"085dac297fffffff0147c020488d610d\",\"subtype\":\"region\",\"name\":\"Cotopaxi\"},{\"division_id\":\"085bf1b2ffffffff012715ee575c3475\",\"subtype\":\"county\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf010ffffffff014f248c7b4fe243\",\"subtype\":\"locality\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf03a3fffffff01145a02d706b16f\",\"subtype\":\"locality\",\"name\":\"Tiliguila\"}]]" +}, "geometry": {"type":"Point","coordinates":[0,0]}} diff --git a/tests/overture-235/out/-z0.json b/tests/overture-235/out/-z0.json new file mode 100644 index 00000000..df00a650 --- /dev/null +++ b/tests/overture-235/out/-z0.json @@ -0,0 +1,26 @@ +{ "type": "FeatureCollection", "properties": { +"antimeridian_adjusted_bounds": "-73.862022,-15.744153,0.000000,51.494860", +"bounds": "-73.862022,-15.744153,0.000000,51.494860", +"center": "0.000000,0.000000,0", +"description": "tests/overture-235/out/-z0.json.check.mbtiles", +"format": "pbf", +"generator_options": "./tippecanoe -q -a@ -f -o tests/overture-235/out/-z0.json.check.mbtiles -z0 tests/overture-235/in.json", +"json": "{\"vector_layers\":[{\"id\":\"in\",\"description\":\"\",\"minzoom\":0,\"maxzoom\":0,\"fields\":{\"@name\":\"String\",\"country\":\"String\",\"hierarchies\":\"String\",\"id\":\"String\",\"local_type\":\"String\",\"names\":\"String\",\"one\":\"String\",\"parent_division_id\":\"String\",\"region\":\"String\",\"sources\":\"String\",\"subtype\":\"String\",\"two\":\"String\",\"update_time\":\"String\",\"version\":\"Number\"}}],\"tilestats\":{\"layerCount\":1,\"layers\":[{\"layer\":\"in\",\"count\":4,\"geometry\":\"Point\",\"attributeCount\":14,\"attributes\":[{\"attribute\":\"@name\",\"count\":2,\"type\":\"string\",\"values\":[\"Andenes\",\"Hammersmith And Fulham\"]},{\"attribute\":\"country\",\"count\":2,\"type\":\"string\",\"values\":[\"GB\",\"PE\"]},{\"attribute\":\"hierarchies\",\"count\":2,\"type\":\"string\",\"values\":[]},{\"attribute\":\"id\",\"count\":2,\"type\":\"string\",\"values\":[\"085070173fffffff014fa86038bb5217\",\"0856c5143fffffff0138e27e7a03f7d8\"]},{\"attribute\":\"local_type\",\"count\":2,\"type\":\"string\",\"values\":[\"{\\\"en\\\":\\\"county\\\"}\",\"{\\\"en\\\":\\\"hamlet\\\"}\"]},{\"attribute\":\"names\",\"count\":2,\"type\":\"string\",\"values\":[\"{\\\"primary\\\":\\\"Andenes\\\",\\\"common\\\":{\\\"en\\\":\\\"Andenes\\\",\\\"es\\\":\\\"Andenes\\\"},\\\"rules\\\":null}\",\"{\\\"primary\\\":\\\"Hammersmith And Fulham\\\",\\\"common\\\":null,\\\"rules\\\":null}\"]},{\"attribute\":\"one\",\"count\":1,\"type\":\"string\",\"values\":[\"Poull Bojer\"]},{\"attribute\":\"parent_division_id\",\"count\":2,\"type\":\"string\",\"values\":[\"085255dd7fffffff011aba243af254bd\",\"0856c53b7fffffff019e8d5b8b047485\"]},{\"attribute\":\"region\",\"count\":2,\"type\":\"string\",\"values\":[\"GB-ENG\",\"PE-ARE\"]},{\"attribute\":\"sources\",\"count\":2,\"type\":\"string\",\"values\":[\"[{\\\"property\\\":\\\"\\\",\\\"dataset\\\":\\\"OpenStreetMap\\\",\\\"record_id\\\":\\\"N3522957503\\\",\\\"confidence\\\":null}]\",\"[{\\\"property\\\":\\\"\\\",\\\"dataset\\\":\\\"geoBoundaries\\\",\\\"record_id\\\":\\\"GBR9080712B6991151074246\\\",\\\"confidence\\\":null}]\"]},{\"attribute\":\"subtype\",\"count\":2,\"type\":\"string\",\"values\":[\"county\",\"locality\"]},{\"attribute\":\"two\",\"count\":1,\"type\":\"string\",\"values\":[]},{\"attribute\":\"update_time\",\"count\":2,\"type\":\"string\",\"values\":[\"2024-05-11T06:56:01Z\",\"2024-05-11T06:57:45Z\"]},{\"attribute\":\"version\",\"count\":1,\"type\":\"number\",\"values\":[0],\"min\":0,\"max\":0}]}]}}", +"maxzoom": "0", +"minzoom": "0", +"name": "tests/overture-235/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", "id": 1096239, "properties": { "id": "085070173fffffff014fa86038bb5217", "@name": "Hammersmith And Fulham", "subtype": "county", "local_type": "{\"en\":\"county\"}", "country": "GB", "region": "GB-ENG", "hierarchies": "[[{\"division_id\":\"085eea613fffffff01a8a15aa3da4c44\",\"subtype\":\"country\",\"name\":\"United Kingdom\"},{\"division_id\":\"085255dd7fffffff011aba243af254bd\",\"subtype\":\"region\",\"name\":\"England\"},{\"division_id\":\"085070173fffffff014fa86038bb5217\",\"subtype\":\"county\",\"name\":\"Hammersmith And Fulham\"}]]", "parent_division_id": "085255dd7fffffff011aba243af254bd", "names": "{\"primary\":\"Hammersmith And Fulham\",\"common\":null,\"rules\":null}", "version": 0, "update_time": "2024-05-11T06:57:45Z", "sources": "[{\"property\":\"\",\"dataset\":\"geoBoundaries\",\"record_id\":\"GBR9080712B6991151074246\",\"confidence\":null}]" }, "geometry": { "type": "Point", "coordinates": [ -0.263672, 51.508742 ] } } +, +{ "type": "Feature", "id": 92526, "properties": { "id": "0856c5143fffffff0138e27e7a03f7d8", "@name": "Andenes", "subtype": "locality", "local_type": "{\"en\":\"hamlet\"}", "country": "PE", "region": "PE-ARE", "hierarchies": "[[{\"division_id\":\"085bc6b4ffffffff01fb3b67a2808435\",\"subtype\":\"country\",\"name\":\"Perú\"},{\"division_id\":\"08559acaffffffff01f27abb0a0cb14b\",\"subtype\":\"region\",\"name\":\"Arequipa\"},{\"division_id\":\"0856c834ffffffff01fd8ac1c98804cb\",\"subtype\":\"county\",\"name\":\"Caravelí\"},{\"division_id\":\"0856c53b7fffffff019e8d5b8b047485\",\"subtype\":\"locality\",\"name\":\"Chaparra\"},{\"division_id\":\"0856c5143fffffff0138e27e7a03f7d8\",\"subtype\":\"locality\",\"name\":\"Andenes\"}]]", "parent_division_id": "0856c53b7fffffff019e8d5b8b047485", "names": "{\"primary\":\"Andenes\",\"common\":{\"en\":\"Andenes\",\"es\":\"Andenes\"},\"rules\":null}", "version": 0, "update_time": "2024-05-11T06:56:01Z", "sources": "[{\"property\":\"\",\"dataset\":\"OpenStreetMap\",\"record_id\":\"N3522957503\",\"confidence\":null}]" }, "geometry": { "type": "Point", "coordinates": [ -73.828125, -15.707663 ] } } +, +{ "type": "Feature", "properties": { "one": "Poull Bojer" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +, +{ "type": "Feature", "properties": { "two": "[[{\"division_id\":\"085ec5c23fffffff01339442745f3d55\",\"subtype\":\"country\",\"name\":\"Ecuador\"},{\"division_id\":\"085dac297fffffff0147c020488d610d\",\"subtype\":\"region\",\"name\":\"Cotopaxi\"},{\"division_id\":\"085bf1b2ffffffff012715ee575c3475\",\"subtype\":\"county\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf010ffffffff014f248c7b4fe243\",\"subtype\":\"locality\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf03a3fffffff01145a02d706b16f\",\"subtype\":\"locality\",\"name\":\"Tiliguila\"}]]" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } } +] } +] } +] } diff --git a/version.hpp b/version.hpp index 30c9ce7c..3871c968 100644 --- a/version.hpp +++ b/version.hpp @@ -1,6 +1,6 @@ #ifndef VERSION_HPP #define VERSION_HPP -#define VERSION "v2.53.0" +#define VERSION "v2.55.0" #endif