Schematic: Properly deal with before/after node resolving and document (#11011)

This fixes an out-of-bounds index access when the node resolver was already applied to the schematic (i.e. biome decoration).
Also improves the handling of the two cases: prior node resolving (m_nodenames), and after node resolving (manual lookup)

2 files changed, 84 insertions, 61 deletions

diff --git a/src/mapgen/mg_schematic.cpp b/src/mapgen/mg_schematic.cpp
index e70e97e48..653bad4fe 100644
--- a/src/mapgen/mg_schematic.cpp
+++ b/src/mapgen/mg_schematic.cpp
@@ -76,10 +76,6 @@ void SchematicManager::clear()
 ///////////////////////////////////////////////////////////////////////////////
 
 
-Schematic::Schematic()
-= default;
-
-
 Schematic::~Schematic()
 {
 	delete []schemdata;
@@ -108,13 +104,19 @@ ObjDef *Schematic::clone() const
 
 void Schematic::resolveNodeNames()
 {
+	c_nodes.clear();
 	getIdsFromNrBacklog(&c_nodes, true, CONTENT_AIR);
 
 	size_t bufsize = size.X * size.Y * size.Z;
 	for (size_t i = 0; i != bufsize; i++) {
 		content_t c_original = schemdata[i].getContent();
-		content_t c_new = c_nodes[c_original];
-		schemdata[i].setContent(c_new);
+		if (c_original >= c_nodes.size()) {
+			errorstream << "Corrupt schematic. name=\"" << name
+				<< "\" at index " << i << std::endl;
+			c_original = 0;
+		}
+		// Unfold condensed ID layout to content_t
+		schemdata[i].setContent(c_nodes[c_original]);
 	}
 }
 
@@ -279,8 +281,7 @@ void Schematic::placeOnMap(ServerMap *map, v3s16 p, u32 flags,
 }
 
 
-bool Schematic::deserializeFromMts(std::istream *is,
-	std::vector<std::string> *names)
+bool Schematic::deserializeFromMts(std::istream *is)
 {
 	std::istream &ss = *is;
 	content_t cignore = CONTENT_IGNORE;
@@ -312,6 +313,8 @@ bool Schematic::deserializeFromMts(std::istream *is,
 		slice_probs[y] = (version >= 3) ? readU8(ss) : MTSCHEM_PROB_ALWAYS_OLD;
 
 	//// Read node names
+	NodeResolver::reset();
+
 	u16 nidmapcount = readU16(ss);
 	for (int i = 0; i != nidmapcount; i++) {
 		std::string name = deSerializeString16(ss);
@@ -324,9 +327,12 @@ bool Schematic::deserializeFromMts(std::istream *is,
 			have_cignore = true;
 		}
 
-		names->push_back(name);
+		m_nodenames.push_back(name);
 	}
 
+	// Prepare for node resolver
+	m_nnlistsizes.push_back(m_nodenames.size());
+
 	//// Read node data
 	size_t nodecount = size.X * size.Y * size.Z;
 
@@ -358,9 +364,11 @@ bool Schematic::deserializeFromMts(std::istream *is,
 }
 
 
-bool Schematic::serializeToMts(std::ostream *os,
-	const std::vector<std::string> &names) const
+bool Schematic::serializeToMts(std::ostream *os) const
 {
+	// Nodes must not be resolved (-> condensed)
+	// checking here is not possible because "schemdata" might be temporary.
+
 	std::ostream &ss = *os;
 
 	writeU32(ss, MTSCHEM_FILE_SIGNATURE);         // signature
@@ -370,9 +378,10 @@ bool Schematic::serializeToMts(std::ostream *os,
 	for (int y = 0; y != size.Y; y++)             // Y slice probabilities
 		writeU8(ss, slice_probs[y]);
 
-	writeU16(ss, names.size()); // name count
-	for (size_t i = 0; i != names.size(); i++)
-		ss << serializeString16(names[i]); // node names
+	writeU16(ss, m_nodenames.size()); // name count
+	for (size_t i = 0; i != m_nodenames.size(); i++) {
+		ss << serializeString16(m_nodenames[i]); // node names
+	}
 
 	// compressed bulk node data
 	MapNode::serializeBulk(ss, SER_FMT_VER_HIGHEST_WRITE,
@@ -382,8 +391,7 @@ bool Schematic::serializeToMts(std::ostream *os,
 }
 
 
-bool Schematic::serializeToLua(std::ostream *os,
-	const std::vector<std::string> &names, bool use_comments,
+bool Schematic::serializeToLua(std::ostream *os, bool use_comments,
 	u32 indent_spaces) const
 {
 	std::ostream &ss = *os;
@@ -392,6 +400,9 @@ bool Schematic::serializeToLua(std::ostream *os,
 	if (indent_spaces > 0)
 		indent.assign(indent_spaces, ' ');
 
+	bool resolve_done = isResolveDone();
+	FATAL_ERROR_IF(resolve_done && !m_ndef, "serializeToLua: NodeDefManager is required");
+
 	//// Write header
 	{
 		ss << "schematic = {" << std::endl;
@@ -436,9 +447,22 @@ bool Schematic::serializeToLua(std::ostream *os,
 				u8 probability   = schemdata[i].param1 & MTSCHEM_PROB_MASK;
 				bool force_place = schemdata[i].param1 & MTSCHEM_FORCE_PLACE;
 
-				ss << indent << indent << "{"
-					<< "name=\"" << names[schemdata[i].getContent()]
-					<< "\", prob=" << (u16)probability * 2
+				// After node resolving: real content_t, lookup using NodeDefManager
+				// Prior node resolving: condensed ID, lookup using m_nodenames
+				content_t c = schemdata[i].getContent();
+
+				ss << indent << indent << "{" << "name=\"";
+
+				if (!resolve_done) {
+					// Prior node resolving (eg. direct schematic load)
+					FATAL_ERROR_IF(c >= m_nodenames.size(), "Invalid node list");
+					ss << m_nodenames[c];
+				} else  {
+					// After node resolving (eg. biome decoration)
+					ss << m_ndef->get(c).name;
+				}
+
+				ss << "\", prob=" << (u16)probability * 2
 					<< ", param2=" << (u16)schemdata[i].param2;
 
 				if (force_place)
@@ -467,25 +491,24 @@ bool Schematic::loadSchematicFromFile(const std::string &filename,
 		return false;
 	}
 
-	size_t origsize = m_nodenames.size();
-	if (!deserializeFromMts(&is, &m_nodenames))
-		return false;
+	if (!m_ndef)
+		m_ndef = ndef;
 
-	m_nnlistsizes.push_back(m_nodenames.size() - origsize);
+	if (!deserializeFromMts(&is))
+		return false;
 
 	name = filename;
 
 	if (replace_names) {
-		for (size_t i = origsize; i < m_nodenames.size(); i++) {
-			std::string &node_name = m_nodenames[i];
+		for (std::string &node_name : m_nodenames) {
 			StringMap::iterator it = replace_names->find(node_name);
 			if (it != replace_names->end())
 				node_name = it->second;
 		}
 	}
 
-	if (ndef)
-		ndef->pendNodeResolve(this);
+	if (m_ndef)
+		m_ndef->pendNodeResolve(this);
 
 	return true;
 }
@@ -494,33 +517,26 @@ bool Schematic::loadSchematicFromFile(const std::string &filename,
 bool Schematic::saveSchematicToFile(const std::string &filename,
 	const NodeDefManager *ndef)
 {
-	MapNode *orig_schemdata = schemdata;
-	std::vector<std::string> ndef_nodenames;
-	std::vector<std::string> *names;
+	Schematic *schem = this;
 
-	if (m_resolve_done && ndef == NULL)
-		ndef = m_ndef;
+	bool needs_condense = isResolveDone();
 
-	if (ndef) {
-		names = &ndef_nodenames;
+	if (!m_ndef)
+		m_ndef = ndef;
 
-		u32 volume = size.X * size.Y * size.Z;
-		schemdata = new MapNode[volume];
-		for (u32 i = 0; i != volume; i++)
-			schemdata[i] = orig_schemdata[i];
+	if (needs_condense) {
+		if (!m_ndef)
+			return false;
 
-		generate_nodelist_and_update_ids(schemdata, volume, names, ndef);
-	} else { // otherwise, use the names we have on hand in the list
-		names = &m_nodenames;
+		schem = (Schematic *)this->clone();
+		schem->condenseContentIds();
 	}
 
 	std::ostringstream os(std::ios_base::binary);
-	bool status = serializeToMts(&os, *names);
+	bool status = schem->serializeToMts(&os);
 
-	if (ndef) {
-		delete []schemdata;
-		schemdata = orig_schemdata;
-	}
+	if (needs_condense)
+		delete schem;
 
 	if (!status)
 		return false;
@@ -556,6 +572,10 @@ bool Schematic::getSchematicFromMap(Map *map, v3s16 p1, v3s16 p2)
 	}
 
 	delete vm;
+
+	// Reset and mark as complete
+	NodeResolver::reset(true);
+
 	return true;
 }
 
@@ -584,26 +604,29 @@ void Schematic::applyProbabilities(v3s16 p0,
 }
 
 
-void generate_nodelist_and_update_ids(MapNode *nodes, size_t nodecount,
-	std::vector<std::string> *usednodes, const NodeDefManager *ndef)
+void Schematic::condenseContentIds()
 {
 	std::unordered_map<content_t, content_t> nodeidmap;
 	content_t numids = 0;
 
+	// Reset node resolve fields
+	NodeResolver::reset();
+
+	size_t nodecount = size.X * size.Y * size.Z;
 	for (size_t i = 0; i != nodecount; i++) {
 		content_t id;
-		content_t c = nodes[i].getContent();
+		content_t c = schemdata[i].getContent();
 
-		std::unordered_map<content_t, content_t>::const_iterator it = nodeidmap.find(c);
+		auto it = nodeidmap.find(c);
 		if (it == nodeidmap.end()) {
 			id = numids;
 			numids++;
 
-			usednodes->push_back(ndef->get(c).name);
-			nodeidmap.insert(std::make_pair(c, id));
+			m_nodenames.push_back(m_ndef->get(c).name);
+			nodeidmap.emplace(std::make_pair(c, id));
 		} else {
 			id = it->second;
 		}
-		nodes[i].setContent(id);
+		schemdata[i].setContent(id);
 	}
 }
diff --git a/src/mapgen/mg_schematic.h b/src/mapgen/mg_schematic.h
index 6b31251b6..5f64ea280 100644
--- a/src/mapgen/mg_schematic.h
+++ b/src/mapgen/mg_schematic.h
@@ -92,7 +92,7 @@ enum SchematicFormatType {
 
 class Schematic : public ObjDef, public NodeResolver {
 public:
-	Schematic();
+	Schematic() = default;
 	virtual ~Schematic();
 
 	ObjDef *clone() const;
@@ -105,11 +105,9 @@ public:
 		const NodeDefManager *ndef);
 	bool getSchematicFromMap(Map *map, v3s16 p1, v3s16 p2);
 
-	bool deserializeFromMts(std::istream *is, std::vector<std::string> *names);
-	bool serializeToMts(std::ostream *os,
-		const std::vector<std::string> &names) const;
-	bool serializeToLua(std::ostream *os, const std::vector<std::string> &names,
-		bool use_comments, u32 indent_spaces) const;
+	bool deserializeFromMts(std::istream *is);
+	bool serializeToMts(std::ostream *os) const;
+	bool serializeToLua(std::ostream *os, bool use_comments, u32 indent_spaces) const;
 
 	void blitToVManip(MMVManip *vm, v3s16 p, Rotation rot, bool force_place);
 	bool placeOnVManip(MMVManip *vm, v3s16 p, u32 flags, Rotation rot, bool force_place);
@@ -124,6 +122,10 @@ public:
 	v3s16 size;
 	MapNode *schemdata = nullptr;
 	u8 *slice_probs = nullptr;
+
+private:
+	// Counterpart to the node resolver: Condense content_t to a sequential "m_nodenames" list
+	void condenseContentIds();
 };
 
 class SchematicManager : public ObjDefManager {
@@ -151,5 +153,3 @@ private:
 	Server *m_server;
 };
 
-void generate_nodelist_and_update_ids(MapNode *nodes, size_t nodecount,
-	std::vector<std::string> *usednodes, const NodeDefManager *ndef);