From ff2ff5c87f500118ac18c1a9df35cc48c5d98d67 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Tue, 25 Jan 2022 04:21:07 -0500 Subject: [PATCH 1/2] fix emplace UB --- include/common/polylib.hh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/common/polylib.hh b/include/common/polylib.hh index ec74114a..a76a4f8c 100644 --- a/include/common/polylib.hh +++ b/include/common/polylib.hh @@ -303,7 +303,7 @@ public: { // move us to dynamic if (count == N) - data.template emplace(begin(), end()); + data = vector_type(begin(), end()); if (is_dynamic()) std::get(data).push_back(vec); @@ -317,8 +317,9 @@ public: { // move us to dynamic if we'll expand too big if (new_size > N && !is_dynamic()) { - auto &vector = data.template emplace(begin(), end()); + auto &vector = vector_type(begin(), end()); vector.resize(new_size); + data = std::move(vector); } else if (is_dynamic()) { if (new_size > N) std::get(data).resize(new_size); From 0414c77c197f91415519df74e3f8f1d3649b8506 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Tue, 25 Jan 2022 04:49:20 -0500 Subject: [PATCH 2/2] Use copies rather than moving for conversion, so that graceful upgrades are graceful again --- common/bspfile.cc | 156 +++++++++++++++++++------------------- include/common/bspfile.hh | 46 +++++++++-- 2 files changed, 117 insertions(+), 85 deletions(-) diff --git a/common/bspfile.cc b/common/bspfile.cc index 03001449..018df768 100644 --- a/common/bspfile.cc +++ b/common/bspfile.cc @@ -995,14 +995,14 @@ static bool BSPVersionSupported(int32_t ident, int32_t version, const bspversion // move structured data if the input and output // are of the same type template -inline void CopyOrMoveArray(T &in, T &out) +inline void CopyArray(T &in, T &out) { - out = std::move(in); + out = in; } // convert structured data if we're different types template>> -inline void CopyOrMoveArray(std::vector &from, std::vector &to) +inline void CopyArray(std::vector &from, std::vector &to) { to.reserve(from.size()); @@ -1010,22 +1010,22 @@ inline void CopyOrMoveArray(std::vector &from, std::vector &to) if constexpr (std::is_arithmetic_v && std::is_arithmetic_v) to.emplace_back(numeric_cast(v)); else - to.emplace_back(std::move(v)); + to.emplace_back(v); } } // move structured data if the input and output // are of the same type template -inline void CopyOrMoveArray(F &in, T &out) +inline void CopyArray(F &in, T &out) { - out = std::move(in); + out = in; } // convert structured data if we're different types // with numeric casting for arrays template>> -inline void CopyOrMoveArray(std::vector> &from, std::vector> &to) +inline void CopyArray(std::vector> &from, std::vector> &to) { to.reserve(from.size()); @@ -1033,7 +1033,7 @@ inline void CopyOrMoveArray(std::vector> &from, std::vector && std::is_arithmetic_v) to.emplace_back(array_cast>(v)); else - to.emplace_back(std::move(v)); + to.emplace_back(v); } } @@ -1041,28 +1041,28 @@ inline void CopyOrMoveArray(std::vector> &from, std::vector inline void ConvertQ1BSPToGeneric(T &bsp, mbsp_t &mbsp) { - CopyOrMoveArray(bsp.dentdata, mbsp.dentdata); - CopyOrMoveArray(bsp.dplanes, mbsp.dplanes); + CopyArray(bsp.dentdata, mbsp.dentdata); + CopyArray(bsp.dplanes, mbsp.dplanes); if (std::holds_alternative(bsp.dtex)) { - CopyOrMoveArray(std::get(bsp.dtex), mbsp.dtex); + CopyArray(std::get(bsp.dtex), mbsp.dtex); } else { - CopyOrMoveArray(std::get(bsp.dtex), mbsp.dtex); + CopyArray(std::get(bsp.dtex), mbsp.dtex); } - CopyOrMoveArray(bsp.dvertexes, mbsp.dvertexes); - CopyOrMoveArray(bsp.dvisdata, mbsp.dvis.bits); - CopyOrMoveArray(bsp.dnodes, mbsp.dnodes); - CopyOrMoveArray(bsp.texinfo, mbsp.texinfo); - CopyOrMoveArray(bsp.dfaces, mbsp.dfaces); - CopyOrMoveArray(bsp.dlightdata, mbsp.dlightdata); - CopyOrMoveArray(bsp.dclipnodes, mbsp.dclipnodes); - CopyOrMoveArray(bsp.dleafs, mbsp.dleafs); - CopyOrMoveArray(bsp.dmarksurfaces, mbsp.dleaffaces); - CopyOrMoveArray(bsp.dedges, mbsp.dedges); - CopyOrMoveArray(bsp.dsurfedges, mbsp.dsurfedges); + CopyArray(bsp.dvertexes, mbsp.dvertexes); + CopyArray(bsp.dvisdata, mbsp.dvis.bits); + CopyArray(bsp.dnodes, mbsp.dnodes); + CopyArray(bsp.texinfo, mbsp.texinfo); + CopyArray(bsp.dfaces, mbsp.dfaces); + CopyArray(bsp.dlightdata, mbsp.dlightdata); + CopyArray(bsp.dclipnodes, mbsp.dclipnodes); + CopyArray(bsp.dleafs, mbsp.dleafs); + CopyArray(bsp.dmarksurfaces, mbsp.dleaffaces); + CopyArray(bsp.dedges, mbsp.dedges); + CopyArray(bsp.dsurfedges, mbsp.dsurfedges); if (std::holds_alternative(bsp.dmodels)) { - CopyOrMoveArray(std::get(bsp.dmodels), mbsp.dmodels); + CopyArray(std::get(bsp.dmodels), mbsp.dmodels); } else { - CopyOrMoveArray(std::get(bsp.dmodels), mbsp.dmodels); + CopyArray(std::get(bsp.dmodels), mbsp.dmodels); } } @@ -1070,24 +1070,24 @@ inline void ConvertQ1BSPToGeneric(T &bsp, mbsp_t &mbsp) template inline void ConvertQ2BSPToGeneric(T &bsp, mbsp_t &mbsp) { - CopyOrMoveArray(bsp.dentdata, mbsp.dentdata); - CopyOrMoveArray(bsp.dplanes, mbsp.dplanes); - CopyOrMoveArray(bsp.dvertexes, mbsp.dvertexes); - CopyOrMoveArray(bsp.dvis, mbsp.dvis); - CopyOrMoveArray(bsp.dnodes, mbsp.dnodes); - CopyOrMoveArray(bsp.texinfo, mbsp.texinfo); - CopyOrMoveArray(bsp.dfaces, mbsp.dfaces); - CopyOrMoveArray(bsp.dlightdata, mbsp.dlightdata); - CopyOrMoveArray(bsp.dleafs, mbsp.dleafs); - CopyOrMoveArray(bsp.dleaffaces, mbsp.dleaffaces); - CopyOrMoveArray(bsp.dleafbrushes, mbsp.dleafbrushes); - CopyOrMoveArray(bsp.dedges, mbsp.dedges); - CopyOrMoveArray(bsp.dsurfedges, mbsp.dsurfedges); - CopyOrMoveArray(bsp.dmodels, mbsp.dmodels); - CopyOrMoveArray(bsp.dbrushes, mbsp.dbrushes); - CopyOrMoveArray(bsp.dbrushsides, mbsp.dbrushsides); - CopyOrMoveArray(bsp.dareas, mbsp.dareas); - CopyOrMoveArray(bsp.dareaportals, mbsp.dareaportals); + CopyArray(bsp.dentdata, mbsp.dentdata); + CopyArray(bsp.dplanes, mbsp.dplanes); + CopyArray(bsp.dvertexes, mbsp.dvertexes); + CopyArray(bsp.dvis, mbsp.dvis); + CopyArray(bsp.dnodes, mbsp.dnodes); + CopyArray(bsp.texinfo, mbsp.texinfo); + CopyArray(bsp.dfaces, mbsp.dfaces); + CopyArray(bsp.dlightdata, mbsp.dlightdata); + CopyArray(bsp.dleafs, mbsp.dleafs); + CopyArray(bsp.dleaffaces, mbsp.dleaffaces); + CopyArray(bsp.dleafbrushes, mbsp.dleafbrushes); + CopyArray(bsp.dedges, mbsp.dedges); + CopyArray(bsp.dsurfedges, mbsp.dsurfedges); + CopyArray(bsp.dmodels, mbsp.dmodels); + CopyArray(bsp.dbrushes, mbsp.dbrushes); + CopyArray(bsp.dbrushsides, mbsp.dbrushsides); + CopyArray(bsp.dareas, mbsp.dareas); + CopyArray(bsp.dareaportals, mbsp.dareaportals); } // Convert from a Q1-esque format to Generic @@ -1097,28 +1097,28 @@ inline T ConvertGenericToQ1BSP(mbsp_t &mbsp, const bspversion_t *to_version) T bsp{}; // copy or convert data - CopyOrMoveArray(mbsp.dentdata, bsp.dentdata); - CopyOrMoveArray(mbsp.dplanes, bsp.dplanes); + CopyArray(mbsp.dentdata, bsp.dentdata); + CopyArray(mbsp.dplanes, bsp.dplanes); if (to_version->game->id == GAME_HALF_LIFE) { - CopyOrMoveArray(mbsp.dtex, bsp.dtex.template emplace()); + CopyArray(mbsp.dtex, bsp.dtex.template emplace()); } else { - CopyOrMoveArray(mbsp.dtex, bsp.dtex.template emplace()); + CopyArray(mbsp.dtex, bsp.dtex.template emplace()); } - CopyOrMoveArray(mbsp.dvertexes, bsp.dvertexes); - CopyOrMoveArray(mbsp.dvis.bits, bsp.dvisdata); - CopyOrMoveArray(mbsp.dnodes, bsp.dnodes); - CopyOrMoveArray(mbsp.texinfo, bsp.texinfo); - CopyOrMoveArray(mbsp.dfaces, bsp.dfaces); - CopyOrMoveArray(mbsp.dlightdata, bsp.dlightdata); - CopyOrMoveArray(mbsp.dclipnodes, bsp.dclipnodes); - CopyOrMoveArray(mbsp.dleafs, bsp.dleafs); - CopyOrMoveArray(mbsp.dleaffaces, bsp.dmarksurfaces); - CopyOrMoveArray(mbsp.dedges, bsp.dedges); - CopyOrMoveArray(mbsp.dsurfedges, bsp.dsurfedges); + CopyArray(mbsp.dvertexes, bsp.dvertexes); + CopyArray(mbsp.dvis.bits, bsp.dvisdata); + CopyArray(mbsp.dnodes, bsp.dnodes); + CopyArray(mbsp.texinfo, bsp.texinfo); + CopyArray(mbsp.dfaces, bsp.dfaces); + CopyArray(mbsp.dlightdata, bsp.dlightdata); + CopyArray(mbsp.dclipnodes, bsp.dclipnodes); + CopyArray(mbsp.dleafs, bsp.dleafs); + CopyArray(mbsp.dleaffaces, bsp.dmarksurfaces); + CopyArray(mbsp.dedges, bsp.dedges); + CopyArray(mbsp.dsurfedges, bsp.dsurfedges); if (to_version->game->id == GAME_HEXEN_II) { - CopyOrMoveArray(mbsp.dmodels, bsp.dmodels.template emplace()); + CopyArray(mbsp.dmodels, bsp.dmodels.template emplace()); } else { - CopyOrMoveArray(mbsp.dmodels, bsp.dmodels.template emplace()); + CopyArray(mbsp.dmodels, bsp.dmodels.template emplace()); } return bsp; @@ -1131,24 +1131,24 @@ inline T ConvertGenericToQ2BSP(mbsp_t &mbsp, const bspversion_t *to_version) T bsp{}; // copy or convert data - CopyOrMoveArray(mbsp.dentdata, bsp.dentdata); - CopyOrMoveArray(mbsp.dplanes, bsp.dplanes); - CopyOrMoveArray(mbsp.dvertexes, bsp.dvertexes); - CopyOrMoveArray(mbsp.dvis, bsp.dvis); - CopyOrMoveArray(mbsp.dnodes, bsp.dnodes); - CopyOrMoveArray(mbsp.texinfo, bsp.texinfo); - CopyOrMoveArray(mbsp.dfaces, bsp.dfaces); - CopyOrMoveArray(mbsp.dlightdata, bsp.dlightdata); - CopyOrMoveArray(mbsp.dleafs, bsp.dleafs); - CopyOrMoveArray(mbsp.dleaffaces, bsp.dleaffaces); - CopyOrMoveArray(mbsp.dleafbrushes, bsp.dleafbrushes); - CopyOrMoveArray(mbsp.dedges, bsp.dedges); - CopyOrMoveArray(mbsp.dsurfedges, bsp.dsurfedges); - CopyOrMoveArray(mbsp.dmodels, bsp.dmodels); - CopyOrMoveArray(mbsp.dbrushes, bsp.dbrushes); - CopyOrMoveArray(mbsp.dbrushsides, bsp.dbrushsides); - CopyOrMoveArray(mbsp.dareas, bsp.dareas); - CopyOrMoveArray(mbsp.dareaportals, bsp.dareaportals); + CopyArray(mbsp.dentdata, bsp.dentdata); + CopyArray(mbsp.dplanes, bsp.dplanes); + CopyArray(mbsp.dvertexes, bsp.dvertexes); + CopyArray(mbsp.dvis, bsp.dvis); + CopyArray(mbsp.dnodes, bsp.dnodes); + CopyArray(mbsp.texinfo, bsp.texinfo); + CopyArray(mbsp.dfaces, bsp.dfaces); + CopyArray(mbsp.dlightdata, bsp.dlightdata); + CopyArray(mbsp.dleafs, bsp.dleafs); + CopyArray(mbsp.dleaffaces, bsp.dleaffaces); + CopyArray(mbsp.dleafbrushes, bsp.dleafbrushes); + CopyArray(mbsp.dedges, bsp.dedges); + CopyArray(mbsp.dsurfedges, bsp.dsurfedges); + CopyArray(mbsp.dmodels, bsp.dmodels); + CopyArray(mbsp.dbrushes, bsp.dbrushes); + CopyArray(mbsp.dbrushsides, bsp.dbrushsides); + CopyArray(mbsp.dareas, bsp.dareas); + CopyArray(mbsp.dareaportals, bsp.dareaportals); return bsp; } diff --git a/include/common/bspfile.hh b/include/common/bspfile.hh index 29bec712..681804dd 100644 --- a/include/common/bspfile.hh +++ b/include/common/bspfile.hh @@ -273,9 +273,41 @@ struct miptex_t std::string name; uint32_t width, height; std::array, MIPLEVELS> data; + + static inline uint8_t *copy_bytes(const uint8_t *in, size_t size) + { + uint8_t *bytes = new uint8_t[size]; + memcpy(bytes, in, size); + return bytes; + } miptex_t() = default; - miptex_t(miptex_t &&) = default; + miptex_t(const miptex_t ©) : + name(copy.name), + width(copy.width), + height(copy.height) + { + for (int32_t i = 0; i < data.size(); i++) { + if (copy.data[i]) { + data[i] = std::unique_ptr(copy_bytes(copy.data[i].get(), (width >> i) * (height >> i))); + } + } + } + + inline miptex_t &operator=(const miptex_t ©) + { + name = copy.name; + width = copy.width; + height = copy.height; + + for (int32_t i = 0; i < data.size(); i++) { + if (copy.data[i]) { + data[i] = std::unique_ptr(copy_bytes(copy.data[i].get(), (width >> i) * (height >> i))); + } + } + + return *this; + } virtual ~miptex_t() { } @@ -343,7 +375,7 @@ struct miptexhl_t : miptex_t miptexhl_t() = default; // convert miptex_t to miptexhl_t - miptexhl_t(miptex_t &&move) : miptex_t(std::forward(move)) { } + miptexhl_t(const miptex_t ©) : miptex_t(copy) { } virtual size_t stream_size() const { @@ -379,14 +411,14 @@ struct dmiptexlump_t dmiptexlump_t() = default; - // move from a different lump type + // copy from a different lump type template - dmiptexlump_t(dmiptexlump_t &&move) + dmiptexlump_t(const dmiptexlump_t ©) { - textures.reserve(move.textures.size()); + textures.reserve(copy.textures.size()); - for (auto &m : move.textures) { - textures.emplace_back(std::move(m)); + for (auto &m : copy.textures) { + textures.emplace_back(m); } }