Fix memory leaks and questionable arithmetic

Fix memory leaks and questionable arithmetic

geojson.c

@@ -79,6 +79,22 @@ struct source {
 	struct source *next;
 };
 
+struct tofree {
+	void *p;
+	struct tofree *next;
+} *tofree = NULL;
+
+void mustfree(void *p) {
+	struct tofree *f = malloc(sizeof(struct tofree));
+	if (f == NULL) {
+		perror("malloc");
+		exit(EXIT_FAILURE);
+	}
+	f->p = p;
+	f->next = tofree;
+	tofree = f;
+}
+
 int CPUS;
 int TEMP_FILES;
 long long MAX_FILES;
@@ -87,12 +103,6 @@ static long long diskfree;
 #define MAX_ZOOM 24
 
 struct reader {
-	char *metaname;
-	char *poolname;
-	char *treename;
-	char *geomname;
-	char *indexname;
-
 	int metafd;
 	int poolfd;
 	int treefd;
@@ -109,7 +119,7 @@ struct reader {
 	long long geompos;
 	long long indexpos;
 
-	long long *file_bbox;
+	long long file_bbox[4];
 
 	struct stat geomst;
 	struct stat metast;
@@ -740,7 +750,8 @@ int serialize_geometry(json_object *geometry, json_object *properties, const cha
 	 */
 	int feature_minzoom = 0;
 	if (mb_geometry[t] == VT_LINE) {
-		for (feature_minzoom = 0; feature_minzoom < 31; feature_minzoom++) {
+		// Skip z0 check because everything is always in the one z0 tile
+		for (feature_minzoom = 1; feature_minzoom < 31; feature_minzoom++) {
 			unsigned mask = 1 << (32 - (feature_minzoom + 1));
 
 			if (((bbox[0] & mask) != (bbox[2] & mask)) || ((bbox[1] & mask) != (bbox[3] & mask))) {
@@ -1585,83 +1596,78 @@ int read_json(int argc, struct source **sourcelist, char *fname, const char *lay
 	for (i = 0; i < CPUS; i++) {
 		struct reader *r = reader + i;
 
-		r->metaname = malloc(strlen(tmpdir) + strlen("/meta.XXXXXXXX") + 1);
+		char metaname[strlen(tmpdir) + strlen("/meta.XXXXXXXX") + 1];
-		r->poolname = malloc(strlen(tmpdir) + strlen("/pool.XXXXXXXX") + 1);
+		char poolname[strlen(tmpdir) + strlen("/pool.XXXXXXXX") + 1];
-		r->treename = malloc(strlen(tmpdir) + strlen("/tree.XXXXXXXX") + 1);
+		char treename[strlen(tmpdir) + strlen("/tree.XXXXXXXX") + 1];
-		r->geomname = malloc(strlen(tmpdir) + strlen("/geom.XXXXXXXX") + 1);
+		char geomname[strlen(tmpdir) + strlen("/geom.XXXXXXXX") + 1];
-		r->indexname = malloc(strlen(tmpdir) + strlen("/index.XXXXXXXX") + 1);
+		char indexname[strlen(tmpdir) + strlen("/index.XXXXXXXX") + 1];
 
-		if (r->metaname == NULL || r->poolname == NULL || r->treename == NULL || r->geomname == NULL || r->indexname == NULL) {
+		sprintf(metaname, "%s%s", tmpdir, "/meta.XXXXXXXX");
-			perror("Out of memory");
+		sprintf(poolname, "%s%s", tmpdir, "/pool.XXXXXXXX");
-			exit(EXIT_FAILURE);
+		sprintf(treename, "%s%s", tmpdir, "/tree.XXXXXXXX");
-		}
+		sprintf(geomname, "%s%s", tmpdir, "/geom.XXXXXXXX");
+		sprintf(indexname, "%s%s", tmpdir, "/index.XXXXXXXX");
 
-		sprintf(r->metaname, "%s%s", tmpdir, "/meta.XXXXXXXX");
+		r->metafd = mkstemp(metaname);
-		sprintf(r->poolname, "%s%s", tmpdir, "/pool.XXXXXXXX");
-		sprintf(r->treename, "%s%s", tmpdir, "/tree.XXXXXXXX");
-		sprintf(r->geomname, "%s%s", tmpdir, "/geom.XXXXXXXX");
-		sprintf(r->indexname, "%s%s", tmpdir, "/index.XXXXXXXX");
-
-		r->metafd = mkstemp(r->metaname);
 		if (r->metafd < 0) {
-			perror(r->metaname);
+			perror(metaname);
 			exit(EXIT_FAILURE);
 		}
-		r->poolfd = mkstemp(r->poolname);
+		r->poolfd = mkstemp(poolname);
 		if (r->poolfd < 0) {
-			perror(r->poolname);
+			perror(poolname);
 			exit(EXIT_FAILURE);
 		}
-		r->treefd = mkstemp(r->treename);
+		r->treefd = mkstemp(treename);
 		if (r->treefd < 0) {
-			perror(r->treename);
+			perror(treename);
 			exit(EXIT_FAILURE);
 		}
-		r->geomfd = mkstemp(r->geomname);
+		r->geomfd = mkstemp(geomname);
 		if (r->geomfd < 0) {
-			perror(r->geomname);
+			perror(geomname);
 			exit(EXIT_FAILURE);
 		}
-		r->indexfd = mkstemp(r->indexname);
+		r->indexfd = mkstemp(indexname);
 		if (r->indexfd < 0) {
-			perror(r->indexname);
+			perror(indexname);
 			exit(EXIT_FAILURE);
 		}
 
-		r->metafile = fopen(r->metaname, "wb");
+		r->metafile = fopen(metaname, "wb");
 		if (r->metafile == NULL) {
-			perror(r->metaname);
+			perror(metaname);
 			exit(EXIT_FAILURE);
 		}
 		r->poolfile = memfile_open(r->poolfd);
 		if (r->poolfile == NULL) {
-			perror(r->poolname);
+			perror(poolname);
 			exit(EXIT_FAILURE);
 		}
 		r->treefile = memfile_open(r->treefd);
 		if (r->treefile == NULL) {
-			perror(r->treename);
+			perror(treename);
 			exit(EXIT_FAILURE);
 		}
-		r->geomfile = fopen(r->geomname, "wb");
+		r->geomfile = fopen(geomname, "wb");
 		if (r->geomfile == NULL) {
-			perror(r->geomname);
+			perror(geomname);
 			exit(EXIT_FAILURE);
 		}
-		r->indexfile = fopen(r->indexname, "wb");
+		r->indexfile = fopen(indexname, "wb");
 		if (r->indexfile == NULL) {
-			perror(r->indexname);
+			perror(indexname);
 			exit(EXIT_FAILURE);
 		}
 		r->metapos = 0;
 		r->geompos = 0;
 		r->indexpos = 0;
 
-		unlink(r->metaname);
+		unlink(metaname);
-		unlink(r->poolname);
+		unlink(poolname);
-		unlink(r->treename);
+		unlink(treename);
-		unlink(r->geomname);
+		unlink(geomname);
-		unlink(r->indexname);
+		unlink(indexname);
 
 		// To distinguish a null value
 		{
@@ -1671,11 +1677,6 @@ int read_json(int argc, struct source **sourcelist, char *fname, const char *lay
 		// Keep metadata file from being completely empty if no attributes
 		serialize_int(r->metafile, 0, &r->metapos, "meta");
 
-		r->file_bbox = malloc(4 * sizeof(long long));
-		if (r->file_bbox == NULL) {
-			perror("Out of memory");
-			exit(EXIT_FAILURE);
-		}
 		r->file_bbox[0] = r->file_bbox[1] = UINT_MAX;
 		r->file_bbox[2] = r->file_bbox[3] = 0;
 	}
@@ -2596,6 +2597,8 @@ int main(int argc, char **argv) {
 				perror("Out of memory");
 				exit(EXIT_FAILURE);
 			}
+			mustfree(src->layer);
+			mustfree(src->file);
 			src->layer[cp - optarg] = '\0';
 			src->next = sources;
 			sources = src;
@@ -2772,6 +2775,21 @@ int main(int argc, char **argv) {
 	files_open_at_start = open("/dev/null", O_RDONLY);
 	close(files_open_at_start);
 
+	if (full_detail <= 0) {
+		full_detail = 12;
+	}
+
+	if (full_detail < min_detail || low_detail < min_detail) {
+		fprintf(stderr, "%s: Full detail and low detail must be at least minimum detail\n", argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	// Need two checks: one for geometry representation, the other for
+	// index traversal when guessing base zoom and drop rate
+	if (maxzoom > 32 - full_detail) {
+		maxzoom = 32 - full_detail;
+		fprintf(stderr, "Highest supported zoom with detail %d is %d\n", full_detail, maxzoom);
+	}
 	if (maxzoom > MAX_ZOOM) {
 		maxzoom = MAX_ZOOM;
 		fprintf(stderr, "Highest supported zoom is %d\n", maxzoom);
@@ -2786,15 +2804,6 @@ int main(int argc, char **argv) {
 		basezoom = maxzoom;
 	}
 
-	if (full_detail <= 0) {
-		full_detail = 12;
-	}
-
-	if (full_detail < min_detail || low_detail < min_detail) {
-		fprintf(stderr, "%s: Full detail and low detail must be at least minimum detail\n", argv[0]);
-		exit(EXIT_FAILURE);
-	}
-
 	geometry_scale = 32 - (full_detail + maxzoom);
 	if (geometry_scale < 0) {
 		geometry_scale = 0;
@@ -2855,5 +2864,19 @@ int main(int argc, char **argv) {
 		exit(EXIT_FAILURE);
 	}
 
+	for (i = 0; i < nsources; i++) {
+		free(sourcelist[i]);
+	}
+
+	pool_free(&exclude);
+	pool_free(&include);
+
+	struct tofree *tf, *next;
+	for (tf = tofree; tf != NULL; tf = next) {
+		next = tf->next;
+		free(tf->p);
+		free(tf);
+	}
+
 	return ret;
 }

geometry.cc

@@ -44,8 +44,8 @@ drawvec decode_geometry(FILE *meta, long long *geompos, int z, unsigned tx, unsi
 			deserialize_long_long_io(meta, &dx, geompos);
 			deserialize_long_long_io(meta, &dy, geompos);
 
-			wx += dx << geometry_scale;
+			wx += dx * (1 << geometry_scale);
-			wy += dy << geometry_scale;
+			wy += dy * (1 << geometry_scale);
 
 			long long wwx = wx;
 			long long wwy = wy;
@@ -302,9 +302,6 @@ static void decode_clipped(ClipperLib::PolyNode *t, drawvec &out) {
 	// guaranteed that at least one point in the polygon is strictly
 	// inside its parent (not on one of its boundary lines).
 
-	// Once we have that, we can run through the outer rings that have
-	// an even number of parents, 
-
 	std::vector<ring> rings;
 	decode_rings(t, rings);
 	std::sort(rings.begin(), rings.end());
@@ -319,10 +316,15 @@ static void decode_clipped(ClipperLib::PolyNode *t, drawvec &out) {
 				}
 			}
 		}
-nextring:
+	nextring:
 		;
 	}
 
+	// Then reverse the winding order of any rings that turned out
+	// to actually be inner when they are outer, or vice versa.
+	// (A ring is outer if it has no parent or if its parent is
+	// an inner ring.)
+
 	for (size_t ii = rings.size(); ii > 0; ii--) {
 		size_t i = ii - 1;
 
@@ -339,6 +341,10 @@ nextring:
 		}
 	}
 
+	// Then run through the rings again, outputting each outer ring
+	// followed by its direct children, and checking to make sure
+	// there are no child rings whose parents weren't identified.
+
 	for (size_t ii = rings.size(); ii > 0; ii--) {
 		size_t i = ii - 1;
 
@@ -535,11 +541,11 @@ void check_polygon(drawvec &geom, drawvec &before) {
 
 				if (t > 0 && t < 1 && s > 0 && s < 1) {
 					printf("Internal error: self-intersecting polygon. %lld,%lld to %lld,%lld intersects %lld,%lld to %lld,%lld\n",
-						geom[i + 0].x, geom[i + 0].y,
+					       geom[i + 0].x, geom[i + 0].y,
-						geom[i + 1].x, geom[i + 1].y,
+					       geom[i + 1].x, geom[i + 1].y,
-						geom[j + 0].x, geom[j + 0].y,
+					       geom[j + 0].x, geom[j + 0].y,
-						geom[j + 1].x, geom[j + 1].y);
+					       geom[j + 1].x, geom[j + 1].y);
-						dump(before);
+					dump(before);
 				}
 			}
 		}

jsonpull.c

@@ -86,6 +86,7 @@ json_pull *json_begin_string(char *s) {
 }
 
 void json_end(json_pull *p) {
+	json_free(p->root);
 	free(p->buffer);
 	free(p);
 }
@@ -102,7 +103,7 @@ static inline int read_wrap(json_pull *j) {
 
 #define SIZE_FOR(i, size) ((size_t)((((i) + 31) & ~31) * size))
 
-static json_object *fabricate_object(json_object *parent, json_type type) {
+static json_object *fabricate_object(json_pull *jp, json_object *parent, json_type type) {
 	json_object *o = malloc(sizeof(struct json_object));
 	if (o == NULL) {
 		perror("Out of memory");
@@ -114,12 +115,13 @@ static json_object *fabricate_object(json_object *parent, json_type type) {
 	o->keys = NULL;
 	o->values = NULL;
 	o->length = 0;
+	o->parser = jp;
 	return o;
 }
 
 static json_object *add_object(json_pull *j, json_type type) {
 	json_object *c = j->container;
-	json_object *o = fabricate_object(c, type);
+	json_object *o = fabricate_object(j, c, type);
 
 	if (c != NULL) {
 		if (c->type == JSON_ARRAY) {
@@ -178,6 +180,10 @@ static json_object *add_object(json_pull *j, json_type type) {
 			}
 		}
 	} else {
+		if (j->root != NULL) {
+			json_free(j->root);
+		}
+
 		j->root = o;
 	}
 
@@ -270,6 +276,10 @@ json_object *json_read_separators(json_pull *j, json_separator_callback cb, void
 
 	// In case there is an error at the top level
 	if (j->container == NULL) {
+		if (j->root != NULL) {
+			json_free(j->root);
+		}
+
 		j->root = NULL;
 	}
 
@@ -662,11 +672,11 @@ void json_disconnect(json_object *o) {
 
 			for (i = 0; i < o->parent->length; i++) {
 				if (o->parent->keys[i] == o) {
-					o->parent->keys[i] = fabricate_object(o->parent, JSON_NULL);
+					o->parent->keys[i] = fabricate_object(o->parser, o->parent, JSON_NULL);
 					break;
 				}
 				if (o->parent->values[i] == o) {
-					o->parent->values[i] = fabricate_object(o->parent, JSON_NULL);
+					o->parent->values[i] = fabricate_object(o->parser, o->parent, JSON_NULL);
 					break;
 				}
 			}
@@ -686,6 +696,10 @@ void json_disconnect(json_object *o) {
 		}
 	}
 
+	if (o->parser != NULL && o->parser->root == o) {
+		o->parser->root = NULL;
+	}
+
 	o->parent = NULL;
 }
 

jsonpull.h

@@ -21,6 +21,7 @@ typedef enum json_type {
 typedef struct json_object {
 	json_type type;
 	struct json_object *parent;
+	struct json_pull *parser;
 
 	char *string;
 	double number;

mvt.cc

@@ -5,6 +5,7 @@
 #include <map>
 #include <zlib.h>
 #include "mvt.hh"
+#include "protozero/varint.hpp"
 #include "protozero/pbf_reader.hpp"
 #include "protozero/pbf_writer.hpp"
 
@@ -78,10 +79,6 @@ int compress(std::string const &input, std::string &output) {
 	return 0;
 }
 
-int dezig(unsigned n) {
-	return (n >> 1) ^ (-(n & 1));
-}
-
 bool mvt_tile::decode(std::string &message) {
 	layers.clear();
 	std::string src;
@@ -213,8 +210,8 @@ bool mvt_tile::decode(std::string &message) {
 
 						if (op == mvt_moveto || op == mvt_lineto) {
 							for (size_t k = 0; k < count && g + 2 < geoms.size(); k++) {
-								px += dezig(geoms[g + 1]);
+								px += protozero::decode_zigzag32(geoms[g + 1]);
-								py += dezig(geoms[g + 2]);
+								py += protozero::decode_zigzag32(geoms[g + 2]);
 								g += 2;
 
 								feature.geometry.push_back(mvt_geometry(op, px, py));
@@ -332,8 +329,8 @@ std::string mvt_tile::encode() {
 					int dx = wwx - px;
 					int dy = wwy - py;
 
-					geometry.push_back((dx << 1) ^ (dx >> 31));
+					geometry.push_back(protozero::encode_zigzag32(dx));
-					geometry.push_back((dy << 1) ^ (dy >> 31));
+					geometry.push_back(protozero::encode_zigzag32(dy));
 
 					px = wwx;
 					py = wwy;

projection.c

@@ -38,11 +38,11 @@ void tile2latlon(long long x, long long y, int zoom, double *lat, double *lon) {
 }
 
 unsigned long long encode(unsigned int wx, unsigned int wy) {
-	long long out = 0;
+	unsigned long long out = 0;
 
 	int i;
 	for (i = 0; i < 32; i++) {
-		long long v = ((wx >> (32 - (i + 1))) & 1) << 1;
+		unsigned long long v = ((wx >> (32 - (i + 1))) & 1) << 1;
 		v |= (wy >> (32 - (i + 1))) & 1;
 		v = v << (64 - 2 * (i + 1));
 

tests/highzoom/out/-z30.json

@@ -1,10 +1,10 @@
 { "type": "FeatureCollection", "properties": {
 "bounds": "-122.000000,37.000000,0.000000,51.500000",
-"center": "-122.000000,37.000008,24",
+"center": "-121.999912,37.000000,20",
 "description": "tests/highzoom/out/-z30.json.check.mbtiles",
 "format": "pbf",
-"json": "{\"vector_layers\": [ { \"id\": \"in\", \"description\": \"\", \"minzoom\": 0, \"maxzoom\": 24, \"fields\": {\"name\": \"String\"} } ] }",
+"json": "{\"vector_layers\": [ { \"id\": \"in\", \"description\": \"\", \"minzoom\": 0, \"maxzoom\": 20, \"fields\": {\"name\": \"String\"} } ] }",
-"maxzoom": "24",
+"maxzoom": "20",
 "minzoom": "0",
 "name": "tests/highzoom/out/-z30.json.check.mbtiles",
 "type": "overlay",
@@ -369,76 +369,4 @@
 { "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 51.500000 ] } }
 ] }
 ] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 21, "x": 337874, "y": 816274 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "Santa Cruz" }, "geometry": { "type": "Point", "coordinates": [ -122.000084, 37.000085 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 21, "x": 1048575, "y": 697425 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ -0.000172, 51.500087 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 21, "x": 1048576, "y": 697425 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 51.500087 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 22, "x": 675748, "y": 1632549 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "Santa Cruz" }, "geometry": { "type": "Point", "coordinates": [ -122.000084, 37.000016 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 22, "x": 2097151, "y": 1394851 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ -0.000086, 51.500034 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 22, "x": 2097152, "y": 1394851 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 51.500034 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 23, "x": 1351497, "y": 3265098 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "Santa Cruz" }, "geometry": { "type": "Point", "coordinates": [ -122.000041, 37.000016 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 23, "x": 4194303, "y": 2789703 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ -0.000043, 51.500007 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 23, "x": 4194304, "y": 2789703 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 51.500007 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 24, "x": 2702995, "y": 6530196 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "Santa Cruz" }, "geometry": { "type": "Point", "coordinates": [ -122.000020, 37.000016 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 24, "x": 8388607, "y": 5579406 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ -0.000021, 51.500007 ] } }
-] }
-] }
-,
-{ "type": "FeatureCollection", "properties": { "zoom": 24, "x": 8388608, "y": 5579406 }, "features": [
-{ "type": "FeatureCollection", "properties": { "layer": "in" }, "features": [
-{ "type": "Feature", "properties": { "name": "London" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 51.500007 ] } }
-] }
-] }
 ] }

tile.cc

@@ -866,7 +866,10 @@ long long write_tile(FILE *geoms, long long *geompos_in, char *metabase, char *s
 			}
 
 			std::vector<coalesce> out;
-			for (size_t x = 0; x < features[j].size(); x++) {
+			if (features[j].size() > 0) {
+				out.push_back(features[j][0]);
+			}
+			for (size_t x = 1; x < features[j].size(); x++) {
 				size_t y = out.size() - 1;
 
 #if 0