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)

diff --git a/src/mapgen/mg_schematic.cpp b/src/mapgen/mg_schematic.cpp
index e70e97e488b3e58517a2b53bdd270b20e3fc522b..653bad4fe5f18c71be1a5cfb428ee7b29f0e9a9c 100644 (file)
--- 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 6b31251b6d5ddbd31d5a364d0d2322010a6143af..5f64ea2803d1893d0a049530623d80c8cea9e3e0 100644 (file)
--- 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 @@ class Schematic : public ObjDef, public NodeResolver {
                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 @@ class Schematic : public ObjDef, public NodeResolver {
        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 @@ class SchematicManager : public ObjDefManager {
        Server *m_server;
 };
 
-void generate_nodelist_and_update_ids(MapNode *nodes, size_t nodecount,
-       std::vector<std::string> *usednodes, const NodeDefManager *ndef);

diff --git a/src/nodedef.cpp b/src/nodedef.cpp
index 57d4c008f4243eef34bf6cfffba7004fc1020b50..8a1f6203b07c9a5470c1a2dfb970a68012d99b6d 100644 (file)
--- a/src/nodedef.cpp
+++ b/src/nodedef.cpp
@@ -1675,8 +1675,7 @@ bool NodeDefManager::nodeboxConnects(MapNode from, MapNode to,
 
 NodeResolver::NodeResolver()
 {
-       m_nodenames.reserve(16);
-       m_nnlistsizes.reserve(4);
+       reset();
 }
 
 
@@ -1779,3 +1778,16 @@ bool NodeResolver::getIdsFromNrBacklog(std::vector<content_t> *result_out,
 
        return success;
 }
+
+void NodeResolver::reset(bool resolve_done)
+{
+       m_nodenames.clear();
+       m_nodenames_idx = 0;
+       m_nnlistsizes.clear();
+       m_nnlistsizes_idx = 0;
+
+       m_resolve_done = resolve_done;
+
+       m_nodenames.reserve(16);
+       m_nnlistsizes.reserve(4);
+}

diff --git a/src/nodedef.h b/src/nodedef.h
index 6fc20518df40acc52f7935fc517e046c0283372b..3e77624eb3134b7d25eca4b003d398005786d642 100644 (file)
--- a/src/nodedef.h
+++ b/src/nodedef.h
@@ -44,6 +44,9 @@ class ITextureSource;
 class IShaderSource;
 class IGameDef;
 class NodeResolver;
+#if BUILD_UNITTESTS
+class TestSchematic;
+#endif
 
 enum ContentParamType
 {
@@ -789,10 +792,13 @@ class NodeDefManager {
 
 NodeDefManager *createNodeDefManager();
 
+// NodeResolver: Queue for node names which are then translated
+// to content_t after the NodeDefManager was initialized
 class NodeResolver {
 public:
        NodeResolver();
        virtual ~NodeResolver();
+       // Callback which is run as soon NodeDefManager is ready
        virtual void resolveNodeNames() = 0;
 
        // required because this class is used as mixin for ObjDef
@@ -804,12 +810,31 @@ class NodeResolver {
        bool getIdsFromNrBacklog(std::vector<content_t> *result_out,
                bool all_required = false, content_t c_fallback = CONTENT_IGNORE);
 
-       void nodeResolveInternal();
+       inline bool isResolveDone() const { return m_resolve_done; }
+       void reset(bool resolve_done = false);
 
-       u32 m_nodenames_idx = 0;
-       u32 m_nnlistsizes_idx = 0;
+       // Vector containing all node names in the resolve "queue"
        std::vector<std::string> m_nodenames;
+       // Specifies the "set size" of node names which are to be processed
+       // this is used for getIdsFromNrBacklog
+       // TODO: replace or remove
        std::vector<size_t> m_nnlistsizes;
+
+protected:
+       friend class NodeDefManager; // m_ndef
+
        const NodeDefManager *m_ndef = nullptr;
+       // Index of the next "m_nodenames" entry to resolve
+       u32 m_nodenames_idx = 0;
+
+private:
+#if BUILD_UNITTESTS
+       // Unittest requires access to m_resolve_done
+       friend class TestSchematic;
+#endif
+       void nodeResolveInternal();
+
+       // Index of the next "m_nnlistsizes" entry to process
+       u32 m_nnlistsizes_idx = 0;
        bool m_resolve_done = false;
 };

diff --git a/src/script/lua_api/l_mapgen.cpp b/src/script/lua_api/l_mapgen.cpp
index 12a497b1e7912a755fe5a6a2bdb74eb09b202f25..cc93bdbd0621e797e0ec1f5e5272f7b9eb21d146 100644 (file)
--- a/src/script/lua_api/l_mapgen.cpp
+++ b/src/script/lua_api/l_mapgen.cpp
@@ -1734,11 +1734,10 @@ int ModApiMapgen::l_serialize_schematic(lua_State *L)
        std::ostringstream os(std::ios_base::binary);
        switch (schem_format) {
        case SCHEM_FMT_MTS:
-               schem->serializeToMts(&os, schem->m_nodenames);
+               schem->serializeToMts(&os);
                break;
        case SCHEM_FMT_LUA:
-               schem->serializeToLua(&os, schem->m_nodenames,
-                       use_comments, indent_spaces);
+               schem->serializeToLua(&os, use_comments, indent_spaces);
                break;
        default:
                return 0;

diff --git a/src/unittest/test_noderesolver.cpp b/src/unittest/test_noderesolver.cpp
index 28da43620926cc9adedbd56714b08d1850e9aaba..ed66093a9e543ea8c28b551d92c3af1d17286550 100644 (file)
--- a/src/unittest/test_noderesolver.cpp
+++ b/src/unittest/test_noderesolver.cpp
@@ -56,6 +56,8 @@ void TestNodeResolver::runTests(IGameDef *gamedef)
 
 class Foobar : public NodeResolver {
 public:
+       friend class TestNodeResolver; // m_ndef
+
        void resolveNodeNames();
 
        content_t test_nr_node1;

diff --git a/src/unittest/test_schematic.cpp b/src/unittest/test_schematic.cpp
index da4ce50d2c39035882c2a681a8af34862ba397a7..d2f027eb4c85c0a5f8bb85fe571acccdc128464b 100644 (file)
--- a/src/unittest/test_schematic.cpp
+++ b/src/unittest/test_schematic.cpp
@@ -66,13 +66,14 @@ void TestSchematic::testMtsSerializeDeserialize(const NodeDefManager *ndef)
        std::stringstream ss(std::ios_base::binary |
                std::ios_base::in | std::ios_base::out);
 
-       std::vector<std::string> names;
-       names.emplace_back("foo");
-       names.emplace_back("bar");
-       names.emplace_back("baz");
-       names.emplace_back("qux");
-
-       Schematic schem, schem2;
+       Schematic schem;
+       {
+               std::vector<std::string> &names = schem.m_nodenames;
+               names.emplace_back("foo");
+               names.emplace_back("bar");
+               names.emplace_back("baz");
+               names.emplace_back("qux");
+       }
 
        schem.flags       = 0;
        schem.size        = size;
@@ -83,18 +84,21 @@ void TestSchematic::testMtsSerializeDeserialize(const NodeDefManager *ndef)
        for (s16 y = 0; y != size.Y; y++)
                schem.slice_probs[y] = MTSCHEM_PROB_ALWAYS;
 
-       UASSERT(schem.serializeToMts(&ss, names));
+       UASSERT(schem.serializeToMts(&ss));
 
        ss.seekg(0);
-       names.clear();
 
-       UASSERT(schem2.deserializeFromMts(&ss, &names));
+       Schematic schem2;
+       UASSERT(schem2.deserializeFromMts(&ss));
 
-       UASSERTEQ(size_t, names.size(), 4);
-       UASSERTEQ(std::string, names[0], "foo");
-       UASSERTEQ(std::string, names[1], "bar");
-       UASSERTEQ(std::string, names[2], "baz");
-       UASSERTEQ(std::string, names[3], "qux");
+       {
+               std::vector<std::string> &names = schem2.m_nodenames;
+               UASSERTEQ(size_t, names.size(), 4);
+               UASSERTEQ(std::string, names[0], "foo");
+               UASSERTEQ(std::string, names[1], "bar");
+               UASSERTEQ(std::string, names[2], "baz");
+               UASSERTEQ(std::string, names[3], "qux");
+       }
 
        UASSERT(schem2.size == size);
        for (size_t i = 0; i != volume; i++)
@@ -120,14 +124,14 @@ void TestSchematic::testLuaTableSerialize(const NodeDefManager *ndef)
        for (s16 y = 0; y != size.Y; y++)
                schem.slice_probs[y] = MTSCHEM_PROB_ALWAYS;
 
-       std::vector<std::string> names;
+       std::vector<std::string> &names = schem.m_nodenames;
        names.emplace_back("air");
        names.emplace_back("default:lava_source");
        names.emplace_back("default:glass");
 
        std::ostringstream ss(std::ios_base::binary);
 
-       UASSERT(schem.serializeToLua(&ss, names, false, 0));
+       UASSERT(schem.serializeToLua(&ss, false, 0));
        UASSERTEQ(std::string, ss.str(), expected_lua_output);
 }
 
@@ -159,6 +163,8 @@ void TestSchematic::testFileSerializeDeserialize(const NodeDefManager *ndef)
        schem1.slice_probs[0] = 80;
        schem1.slice_probs[1] = 160;
        schem1.slice_probs[2] = 240;
+       // Node resolving happened manually.
+       schem1.m_resolve_done = true;
 
        for (size_t i = 0; i != volume; i++) {
                content_t c = content_map[test_schem2_data[i]];