Keep track of decoded/rendered blocks

and fix small oversight in previous commit

src/BlockDecoder.cpp

@@ -1,6 +1,5 @@
 #include <string>
-#include <iostream>
+#include <utility>
-#include <sstream>
 
 #include "BlockDecoder.h"
 #include "ZlibDecompressor.h"
@@ -46,13 +45,12 @@ void BlockDecoder::decode(const ustring &datastr)
 {
 	const unsigned char *data = datastr.c_str();
 	size_t length = datastr.length();
-	// TODO: bounds checks
+	// TODO: Add strict bounds checks everywhere
 
 	uint8_t version = data[0];
 	if (version < 22) {
-		std::ostringstream oss;
+		auto err = "Unsupported map version " + std::to_string(version);
-		oss << "Unsupported map version " << (int)version;
+		throw std::runtime_error(err);
-		throw std::runtime_error(oss.str());
 	}
 	m_version = version;
 
@@ -87,7 +85,7 @@ void BlockDecoder::decode(const ustring &datastr)
 			else if (name == "ignore")
 				m_blockIgnoreId = nodeId;
 			else
-				m_nameMap[nodeId] = name;
+				m_nameMap[nodeId] = std::move(name);
 			dataOffset += nameLen;
 		}
 	};
@@ -99,14 +97,20 @@ void BlockDecoder::decode(const ustring &datastr)
 	dataOffset++;
 	uint8_t paramsWidth = data[dataOffset];
 	dataOffset++;
-	if (contentWidth != 1 && contentWidth != 2)
+	if (contentWidth != 1 && contentWidth != 2) {
-		throw std::runtime_error("unsupported map version (contentWidth)");
+		auto err = "Unsupported map version contentWidth=" + std::to_string(contentWidth);
-	if (paramsWidth != 2)
+		throw std::runtime_error(err);
-		throw std::runtime_error("unsupported map version (paramsWidth)");
+	}
+	if (paramsWidth != 2) {
+		auto err = "Unsupported map version paramsWidth=" + std::to_string(paramsWidth);
+		throw std::runtime_error(err);
+	}
 	m_contentWidth = contentWidth;
+	const size_t mapDataSize = (contentWidth + paramsWidth) * 4096;
 
 	if (version >= 29) {
-		size_t mapDataSize = (contentWidth + paramsWidth) * 4096;
+		if (length < dataOffset + mapDataSize)
+			throw std::runtime_error("Map data buffer truncated");
 		m_mapData.assign(data + dataOffset, mapDataSize);
 		return; // we have read everything we need and can return early
 	}
@@ -118,6 +122,9 @@ void BlockDecoder::decode(const ustring &datastr)
 	decompressor.decompress(m_scratch); // unused metadata
 	dataOffset = decompressor.seekPos();
 
+	if (m_mapData.size() < mapDataSize)
+		throw std::runtime_error("Map data buffer truncated");
+
 	// Skip unused node timers
 	if (version == 23)
 		dataOffset += 1;
@@ -132,7 +139,7 @@ void BlockDecoder::decode(const ustring &datastr)
 
 	// Skip unused static objects
 	dataOffset++; // Skip static object version
-	int staticObjectCount = readU16(data + dataOffset);
+	uint16_t staticObjectCount = readU16(data + dataOffset);
 	dataOffset += 2;
 	for (int i = 0; i < staticObjectCount; ++i) {
 		dataOffset += 13;
@@ -161,7 +168,7 @@ const std::string &BlockDecoder::getNode(u8 x, u8 y, u8 z) const
 		return empty;
 	NameMap::const_iterator it = m_nameMap.find(content);
 	if (it == m_nameMap.end()) {
-		errorstream << "Skipping node with invalid ID." << std::endl;
+		errorstream << "Skipping node with invalid ID " << (int)content << std::endl;
 		return empty;
 	}
 	return it->second;

src/TileGenerator.cpp

@@ -544,9 +544,25 @@ void TileGenerator::renderMap()
 	BlockDecoder blk;
 	const int16_t yMax = mod16(m_yMax) + 1;
 	const int16_t yMin = mod16(m_yMin);
-	size_t count = 0;
+	size_t bTotal = 0, bRender = 0, bEmpty = 0;
+
+	// returns true to skip
+	auto decode = [&] (BlockPos pos, const ustring &buf) -> bool {
+		blk.reset();
+		try {
+			blk.decode(buf);
+		} catch (std::exception &e) {
+			errorstream << "While decoding block " << pos.x << ',' << pos.y << ',' << pos.z
+				<< ':' << std::endl;
+			throw;
+		};
+		return blk.isEmpty();
+	};
 
 	auto renderSingle = [&] (int16_t xPos, int16_t zPos, BlockList &blockStack) {
+		if (blockStack.empty())
+			return;
+
 		m_readPixels.reset();
 		m_readInfo.reset();
 		for (int i = 0; i < 16; i++) {
@@ -557,15 +573,17 @@ void TileGenerator::renderMap()
 			}
 		}
 
+		bTotal += blockStack.size();
 		for (const auto &it : blockStack) {
 			const BlockPos pos = it.first;
 			assert(pos.x == xPos && pos.z == zPos);
 			assert(pos.y >= yMin && pos.y < yMax);
 
-			blk.reset();
+			if (decode(pos, it.second)) {
-			blk.decode(it.second);
+				bEmpty++;
-			if (blk.isEmpty())
 				continue;
+			}
+			bRender++;
 			renderMapBlock(blk, pos);
 
 			// Exit out if all pixels for this MapBlock are covered
@@ -581,6 +599,7 @@ void TileGenerator::renderMap()
 			renderShading(zPos);
 	};
 
+	size_t count = 0; // fraction of m_progressMax
 	if (m_exhaustiveSearch == EXH_NEVER) {
 		for (auto it = m_positions.rbegin(); it != m_positions.rend(); ++it) {
 			int16_t zPos = it->first;
@@ -646,6 +665,8 @@ void TileGenerator::renderMap()
 	}
 
 	reportProgress(m_progressMax);
+	verbosestream << "Block stats: " << bTotal << " total, " << bRender
+		<< " rendered, " << bEmpty << " empty" << std::endl;
 }
 
 void TileGenerator::renderMapBlock(const BlockDecoder &blk, const BlockPos &pos)

src/ZlibDecompressor.h

@@ -6,7 +6,12 @@
 class ZlibDecompressor
 {
 public:
-	class DecompressError : std::exception {};
+	class DecompressError : public std::exception {
+	public:
+		const char* what() const noexcept override {
+			return "ZlibDecompressor::DecompressError";
+		}
+	};
 
 	ZlibDecompressor(const u8 *data, size_t size);
 	~ZlibDecompressor();

src/ZstdDecompressor.h

@@ -6,7 +6,12 @@
 class ZstdDecompressor
 {
 public:
-	class DecompressError : std::exception {};
+	class DecompressError : public std::exception {
+	public:
+		const char* what() const noexcept override {
+			return "ZstdDecompressor::DecompressError";
+		}
+	};
 
 	ZstdDecompressor();
 	~ZstdDecompressor();

src/mapper.cpp

@@ -274,12 +274,12 @@ int main(int argc, char *argv[])
 			return 0;
 		}
 
-		if(colors.empty())
+		if (colors.empty())
 			colors = search_colors(input);
 		generator.parseColorsFile(colors);
 		generator.generate(input, output);
 
-	} catch (const std::exception &e) {
+	} catch (std::exception &e) {
 		errorstream << "Exception: " << e.what() << std::endl;
 		return 1;
 	}

util/ci/test.sh

@@ -13,7 +13,7 @@ encodepos () {
 	echo "$(($1 + 0x1000 * $2 + 0x1000000 * $3))"
 }
 
-# create map file with sql statements
+# create map file using SQL statements
 writemap () {
 	rm -rf $mapdir
 	mkdir $mapdir
@@ -42,6 +42,17 @@ checkmap () {
 	echo "Passed."
 }
 
+# check that invocation returned an error
+checkerr () {
+	local r=0
+	./minetestmapper --noemptyimage -v -i ./testmap -o map.png "$@" || r=1
+	if [ $r -eq 0 ]; then
+		echo "Did not return error!"
+		exit 1
+	fi
+	echo "Passed."
+}
+
 # this is missing the indices and primary keys but that doesn't matter
 schema_old="CREATE TABLE blocks(pos INT, data BLOB);"
 schema_new="CREATE TABLE blocks(x INT, y INT, z INT, data BLOB);"
@@ -60,7 +71,9 @@ checkmap 0 --max-y 16
 checkmap 1 --min-y 18
 checkmap 0 --min-y 19
 
-msg "old schema: all limits"
+# do this for every strategy
+for exh in never y full; do
+msg "old schema: all limits ($exh)"
 # fill the map with more blocks and then request just a single one to be rendered
 # this will run through internal consistency asserts.
 writemap "
@@ -73,7 +86,8 @@ INSERT INTO blocks SELECT $(encodepos 3 2 2), d FROM d;
 INSERT INTO blocks SELECT $(encodepos 2 3 2), d FROM d;
 INSERT INTO blocks SELECT $(encodepos 2 2 3), d FROM d;
 "
-checkmap 1 --geometry 32:32+16+16 --min-y 32 --max-y $((32+16-1))
+checkmap 1 --geometry 32:32+16+16 --min-y 32 --max-y $((32+16-1)) --exhaustive $exh
+done
 
 msg "new schema"
 writemap "
@@ -82,8 +96,9 @@ INSERT INTO blocks SELECT 0, 1, 0, d FROM d;
 "
 checkmap 1
 
-msg "new schema: all limits"
 # same as above
+for exh in never y full; do
+msg "new schema: all limits ($exh)"
 writemap "
 $schema_new
 INSERT INTO blocks SELECT 2, 2, 2, d FROM d;
@@ -94,7 +109,8 @@ INSERT INTO blocks SELECT 3, 2, 2, d FROM d;
 INSERT INTO blocks SELECT 2, 3, 2, d FROM d;
 INSERT INTO blocks SELECT 2, 2, 3, d FROM d;
 "
-checkmap 1 --geometry 32:32+16+16 --min-y 32 --max-y $((32+16-1))
+checkmap 1 --geometry 32:32+16+16 --min-y 32 --max-y $((32+16-1)) --exhaustive $exh
+done
 
 msg "new schema: empty map"
 writemap "$schema_new"
@@ -109,3 +125,17 @@ mkdir $mapdir/players
 printf '%s\n' "name = cat" "position = (80,0,80)" >$mapdir/players/cat
 # we can't check that it actually worked, however
 checkmap 1 --drawplayers --zoom 4
+
+msg "block error (wrong version)"
+writemap "
+$schema_new
+INSERT INTO blocks VALUES (0, 0, 0, x'150000');
+"
+checkerr
+
+msg "block error (invalid zstd)"
+writemap "
+$schema_new
+INSERT INTO blocks VALUES (0, 0, 0, x'1d28b52ffd2001090000');
+"
+checkerr