From 001be98a88e5e32adc1429cea6f6a44df17200e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 31 Jan 2022 17:17:41 +0100 Subject: [PATCH] Trade: don't reinterpret no-op function pointers. It was a clever harmless trick. Well, it was way more harmless than it was clever, but even then it caused UBSan to complain. And that's Not A Good Thing for various reasons, so let's just comply. The main bad effect of this change is a *slightly* larger list of exported symbols but until we actually get rid of the major bloats like , and the like, this is not going to have any measurable impact. --- doc/changelog.dox | 4 +++ src/Magnum/Trade/AbstractImporter.cpp | 32 +++++++++---------- src/Magnum/Trade/AbstractSceneConverter.cpp | 8 ++--- src/Magnum/Trade/Data.cpp | 9 +++++- src/Magnum/Trade/Data.h | 21 +++++++++++- src/Magnum/Trade/MaterialData.cpp | 2 +- src/Magnum/Trade/MeshData.cpp | 3 +- src/Magnum/Trade/SceneData.cpp | 3 +- src/Magnum/Trade/SkinData.cpp | 2 +- .../Trade/Test/AbstractImporterTest.cpp | 2 +- 10 files changed, 57 insertions(+), 29 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index db29fea1d..b8dec0562 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -493,6 +493,10 @@ See also: with filtering along Z or if it's a 2D array with discrete slices. - @ref magnum-imageconverter "magnum-imageconverter" has a new `--in-place` option for converting images in-place +- In order to reduce the amount of exported symbols, a single no-op + @relativeref{Corrade,Containers::Array} deleter function was used for + various types via a @cpp reinterpret_cast @ce. But in an effort to be + UBSan-clean, this is no longer done. See also [mosra/magnum#531](https://github.com/mosra/magnum/issues/531). @subsubsection changelog-latest-changes-vk Vk library diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index 331ae9dd6..398375a64 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -330,8 +330,8 @@ Containers::Optional AbstractImporter::scene(const UnsignedInt id) { CORRADE_ASSERT(id < doSceneCount(), "Trade::AbstractImporter::scene(): index" << id << "out of range for" << doSceneCount() << "entries", {}); Containers::Optional scene = doScene(id); CORRADE_ASSERT(!scene || ( - (!scene->_data.deleter() || scene->_data.deleter() == Implementation::nonOwnedArrayDeleter) && - (!scene->_fields.deleter() || scene->_fields.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + (!scene->_data.deleter() || scene->_data.deleter() == static_cast(Implementation::nonOwnedArrayDeleter)) && + (!scene->_fields.deleter() || scene->_fields.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::scene(): implementation is not allowed to use a custom Array deleter", {}); return scene; } @@ -399,8 +399,8 @@ Containers::Optional AbstractImporter::animation(const UnsignedIn CORRADE_ASSERT(id < doAnimationCount(), "Trade::AbstractImporter::animation(): index" << id << "out of range for" << doAnimationCount() << "entries", {}); Containers::Optional animation = doAnimation(id); CORRADE_ASSERT(!animation || - ((!animation->_data.deleter() || animation->_data.deleter() == Implementation::nonOwnedArrayDeleter || animation->_data.deleter() == ArrayAllocator::deleter) && - (!animation->_tracks.deleter() || animation->_tracks.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + ((!animation->_data.deleter() || animation->_data.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || animation->_data.deleter() == ArrayAllocator::deleter) && + (!animation->_tracks.deleter() || animation->_tracks.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::animation(): implementation is not allowed to use a custom Array deleter", {}); return animation; } @@ -953,8 +953,8 @@ Containers::Optional AbstractImporter::skin2D(const UnsignedInt id) CORRADE_ASSERT(id < doSkin2DCount(), "Trade::AbstractImporter::skin2D(): index" << id << "out of range for" << doSkin2DCount() << "entries", {}); Containers::Optional skin = doSkin2D(id); CORRADE_ASSERT(!skin || ( - (!skin->_jointData.deleter() || skin->_jointData.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter)) && - (!skin->_inverseBindMatrixData.deleter() || skin->_inverseBindMatrixData.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + (!skin->_jointData.deleter() || skin->_jointData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter)) && + (!skin->_inverseBindMatrixData.deleter() || skin->_inverseBindMatrixData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::skin2D(): implementation is not allowed to use a custom Array deleter", {}); return skin; } @@ -1003,8 +1003,8 @@ Containers::Optional AbstractImporter::skin3D(const UnsignedInt id) CORRADE_ASSERT(id < doSkin3DCount(), "Trade::AbstractImporter::skin3D(): index" << id << "out of range for" << doSkin3DCount() << "entries", {}); Containers::Optional skin = doSkin3D(id); CORRADE_ASSERT(!skin || ( - (!skin->_jointData.deleter() || skin->_jointData.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter)) && - (!skin->_inverseBindMatrixData.deleter() || skin->_inverseBindMatrixData.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + (!skin->_jointData.deleter() || skin->_jointData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter)) && + (!skin->_inverseBindMatrixData.deleter() || skin->_inverseBindMatrixData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::skin3D(): implementation is not allowed to use a custom Array deleter", {}); return skin; } @@ -1074,9 +1074,9 @@ Containers::Optional AbstractImporter::mesh(const UnsignedInt id, cons #endif Containers::Optional mesh = doMesh(id, level); CORRADE_ASSERT(!mesh || ( - (!mesh->_indexData.deleter() || mesh->_indexData.deleter() == Implementation::nonOwnedArrayDeleter || mesh->_indexData.deleter() == ArrayAllocator::deleter) && - (!mesh->_vertexData.deleter() || mesh->_vertexData.deleter() == Implementation::nonOwnedArrayDeleter || mesh->_vertexData.deleter() == ArrayAllocator::deleter) && - (!mesh->_attributes.deleter() || mesh->_attributes.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + (!mesh->_indexData.deleter() || mesh->_indexData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || mesh->_indexData.deleter() == ArrayAllocator::deleter) && + (!mesh->_vertexData.deleter() || mesh->_vertexData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || mesh->_vertexData.deleter() == ArrayAllocator::deleter) && + (!mesh->_attributes.deleter() || mesh->_attributes.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::mesh(): implementation is not allowed to use a custom Array deleter", {}); return mesh; } @@ -1240,8 +1240,8 @@ AbstractImporter::material(const UnsignedInt id) { Containers::Optional material = doMaterial(id); CORRADE_ASSERT(!material || ( - (!material->_data.deleter() || material->_data.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter)) && - (!material->_layerOffsets.deleter() || material->_layerOffsets.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + (!material->_data.deleter() || material->_data.deleter() == static_cast(Implementation::nonOwnedArrayDeleter)) && + (!material->_layerOffsets.deleter() || material->_layerOffsets.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::material(): implementation is not allowed to use a custom Array deleter", {}); /* GCC 4.8 and clang-cl needs an explicit conversion here */ @@ -1367,7 +1367,7 @@ Containers::Optional AbstractImporter::image1D(const UnsignedInt id } #endif Containers::Optional image = doImage1D(id, level); - CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == Implementation::nonOwnedArrayDeleter || image->_data.deleter() == ArrayAllocator::deleter, "Trade::AbstractImporter::image1D(): implementation is not allowed to use a custom Array deleter", {}); + CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || image->_data.deleter() == ArrayAllocator::deleter, "Trade::AbstractImporter::image1D(): implementation is not allowed to use a custom Array deleter", {}); return image; } @@ -1437,7 +1437,7 @@ Containers::Optional AbstractImporter::image2D(const UnsignedInt id } #endif Containers::Optional image = doImage2D(id, level); - CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == Implementation::nonOwnedArrayDeleter || image->_data.deleter() == ArrayAllocator::deleter, "Trade::AbstractImporter::image2D(): implementation is not allowed to use a custom Array deleter", {}); + CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || image->_data.deleter() == ArrayAllocator::deleter, "Trade::AbstractImporter::image2D(): implementation is not allowed to use a custom Array deleter", {}); return image; } @@ -1507,7 +1507,7 @@ Containers::Optional AbstractImporter::image3D(const UnsignedInt id } #endif Containers::Optional image = doImage3D(id, level); - CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == Implementation::nonOwnedArrayDeleter || image->_data.deleter() == ArrayAllocator::deleter, "Trade::AbstractImporter::image3D(): implementation is not allowed to use a custom Array deleter", {}); + CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || image->_data.deleter() == ArrayAllocator::deleter, "Trade::AbstractImporter::image3D(): implementation is not allowed to use a custom Array deleter", {}); return image; } diff --git a/src/Magnum/Trade/AbstractSceneConverter.cpp b/src/Magnum/Trade/AbstractSceneConverter.cpp index 7af8a68cf..b7366beee 100644 --- a/src/Magnum/Trade/AbstractSceneConverter.cpp +++ b/src/Magnum/Trade/AbstractSceneConverter.cpp @@ -105,9 +105,9 @@ Containers::Optional AbstractSceneConverter::convert(const MeshData& m Containers::Optional out = doConvert(mesh); CORRADE_ASSERT(!out || ( - (!out->_indexData.deleter() || out->_indexData.deleter() == Implementation::nonOwnedArrayDeleter || out->_indexData.deleter() == ArrayAllocator::deleter) && - (!out->_vertexData.deleter() || out->_vertexData.deleter() == Implementation::nonOwnedArrayDeleter || out->_vertexData.deleter() == ArrayAllocator::deleter) && - (!out->_attributes.deleter() || out->_attributes.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + (!out->_indexData.deleter() || out->_indexData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || out->_indexData.deleter() == ArrayAllocator::deleter) && + (!out->_vertexData.deleter() || out->_vertexData.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || out->_vertexData.deleter() == ArrayAllocator::deleter) && + (!out->_attributes.deleter() || out->_attributes.deleter() == static_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractSceneConverter::convert(): implementation is not allowed to use a custom Array deleter", {}); return out; } @@ -132,7 +132,7 @@ Containers::Array AbstractSceneConverter::convertToData(const MeshData& me "Trade::AbstractSceneConverter::convertToData(): mesh conversion not supported", {}); Containers::Array out = doConvertToData(mesh); - CORRADE_ASSERT(!out || !out.deleter() || out.deleter() == Implementation::nonOwnedArrayDeleter || out.deleter() == ArrayAllocator::deleter, + CORRADE_ASSERT(!out || !out.deleter() || out.deleter() == static_cast(Implementation::nonOwnedArrayDeleter) || out.deleter() == ArrayAllocator::deleter, "Trade::AbstractSceneConverter::convertToData(): implementation is not allowed to use a custom Array deleter", {}); return out; } diff --git a/src/Magnum/Trade/Data.cpp b/src/Magnum/Trade/Data.cpp index 65f6ab3ff..9c4987300 100644 --- a/src/Magnum/Trade/Data.cpp +++ b/src/Magnum/Trade/Data.cpp @@ -53,7 +53,14 @@ Debug& operator<<(Debug& debug, const DataFlags value) { } namespace Implementation { - void nonOwnedArrayDeleter(char*, std::size_t) { /* does nothing */ } + void nonOwnedArrayDeleter(char*, std::size_t) {} + void nonOwnedArrayDeleter(AnimationTrackData*, std::size_t) {} + void nonOwnedArrayDeleter(MeshAttributeData*, std::size_t) {} + void nonOwnedArrayDeleter(MaterialAttributeData*, std::size_t) {} + void nonOwnedArrayDeleter(UnsignedInt*, std::size_t) {} + void nonOwnedArrayDeleter(Matrix3*, std::size_t) {} + void nonOwnedArrayDeleter(Matrix4*, std::size_t) {} + void nonOwnedArrayDeleter(SceneFieldData*, std::size_t) {} } }} diff --git a/src/Magnum/Trade/Data.h b/src/Magnum/Trade/Data.h index 22a5b8203..2a7d3b9db 100644 --- a/src/Magnum/Trade/Data.h +++ b/src/Magnum/Trade/Data.h @@ -33,6 +33,7 @@ #include #include "Magnum/Magnum.h" +#include "Magnum/Trade/Trade.h" #include "Magnum/Trade/visibility.h" namespace Magnum { namespace Trade { @@ -102,8 +103,26 @@ CORRADE_ENUMSET_OPERATORS(DataFlags) MAGNUM_TRADE_EXPORT Debug& operator<<(Debug& debug, DataFlags value); namespace Implementation { - /* Used internally by MeshData */ + /* Used internally by AnimationData, MaterialData, MeshData, SceneData, + SkinData -- we need them to be exported symbols in the Trade library and + not e.g. lambdas in order to ensure that data originating from + dynamically-loaded plugins don't contain pointers to deleter functions + contained inside the plugin binary, leading to a dangling function + pointer call if the array gets destructed after the plugin was unloaded. + + All variants below do the same (which is nothing), but because + reinterpret_cast<>'ing function pointers triggers a UBSan complaint, + they are separate symbols. On MSVC /OPT:ICF *could* merge them, on other + compilers it might result in unnecessary duplicated symbols, but being + UBSan-clean is the bigger priority here. */ MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(char*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(AnimationTrackData*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(MeshAttributeData*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(MaterialAttributeData*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(UnsignedInt*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(Matrix3*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(Matrix4*, std::size_t); + MAGNUM_TRADE_EXPORT void nonOwnedArrayDeleter(SceneFieldData*, std::size_t); } }} diff --git a/src/Magnum/Trade/MaterialData.cpp b/src/Magnum/Trade/MaterialData.cpp index 6ed6d1e54..b939dff43 100644 --- a/src/Magnum/Trade/MaterialData.cpp +++ b/src/Magnum/Trade/MaterialData.cpp @@ -256,7 +256,7 @@ MaterialData::MaterialData(const MaterialTypes types, Containers::Array attributeData, const std::initializer_list layerData, const void* const importerState): MaterialData{types, Implementation::initializerListToArrayWithDefaultDeleter(attributeData), Implementation::initializerListToArrayWithDefaultDeleter(layerData), importerState} {} -MaterialData::MaterialData(const MaterialTypes types, const DataFlags attributeDataFlags, const Containers::ArrayView attributeData, const DataFlags layerDataFlags, Containers::ArrayView layerData, const void* const importerState) noexcept: _data{Containers::Array{const_cast(attributeData.data()), attributeData.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}}, _layerOffsets{Containers::Array{const_cast(layerData.data()), layerData.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}}, _types{types}, _importerState{importerState} { +MaterialData::MaterialData(const MaterialTypes types, const DataFlags attributeDataFlags, const Containers::ArrayView attributeData, const DataFlags layerDataFlags, Containers::ArrayView layerData, const void* const importerState) noexcept: _data{Containers::Array{const_cast(attributeData.data()), attributeData.size(), Implementation::nonOwnedArrayDeleter}}, _layerOffsets{Containers::Array{const_cast(layerData.data()), layerData.size(), Implementation::nonOwnedArrayDeleter}}, _types{types}, _importerState{importerState} { CORRADE_ASSERT(!(attributeDataFlags & DataFlag::Owned), "Trade::MaterialData: can't construct with non-owned attribute data but" << attributeDataFlags, ); CORRADE_ASSERT(!(layerDataFlags & DataFlag::Owned), diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 24e956468..bcdb9f07a 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -79,8 +79,7 @@ MeshAttributeData::MeshAttributeData(const MeshAttribute name, const VertexForma } Containers::Array meshAttributeDataNonOwningArray(const Containers::ArrayView view) { - /* Ugly, eh? */ - return Containers::Array{const_cast(view.data()), view.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}; + return Containers::Array{const_cast(view.data()), view.size(), Implementation::nonOwnedArrayDeleter}; } MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& indexData, const MeshIndexData& indices, Containers::Array&& vertexData, Containers::Array&& attributes, const UnsignedInt vertexCount, const void* const importerState) noexcept: diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index f18f7ee2e..b5fae11ad 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -523,8 +523,7 @@ SceneFieldData::SceneFieldData(const SceneField name, const Containers::StridedA } Containers::Array sceneFieldDataNonOwningArray(const Containers::ArrayView view) { - /* Ugly, eh? */ - return Containers::Array{const_cast(view.data()), view.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}; + return Containers::Array{const_cast(view.data()), view.size(), Implementation::nonOwnedArrayDeleter}; } SceneData::SceneData(const SceneMappingType mappingType, const UnsignedLong mappingBound, Containers::Array&& data, Containers::Array&& fields, const void* const importerState) noexcept: _dataFlags{DataFlag::Owned|DataFlag::Mutable}, _mappingType{mappingType}, _dimensions{}, _mappingBound{mappingBound}, _importerState{importerState}, _fields{std::move(fields)}, _data{std::move(data)} { diff --git a/src/Magnum/Trade/SkinData.cpp b/src/Magnum/Trade/SkinData.cpp index 797cea200..f8dee9350 100644 --- a/src/Magnum/Trade/SkinData.cpp +++ b/src/Magnum/Trade/SkinData.cpp @@ -39,7 +39,7 @@ template SkinData::SkinData(Containers::Arra template SkinData::SkinData(const std::initializer_list joints, const std::initializer_list> inverseBindMatrices, const void* const importerState): SkinData{Implementation::initializerListToArrayWithDefaultDeleter(joints), Implementation::initializerListToArrayWithDefaultDeleter(inverseBindMatrices), importerState} {} -template SkinData::SkinData(DataFlags, const Containers::ArrayView jointData, DataFlags, const Containers::ArrayView> inverseBindMatrixData, const void* const importerState) noexcept: SkinData{Containers::Array{const_cast(jointData.data()), jointData.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}, Containers::Array>{const_cast*>(inverseBindMatrixData.data()), inverseBindMatrixData.size(), reinterpret_cast*, std::size_t)>(Implementation::nonOwnedArrayDeleter)}, importerState} {} +template SkinData::SkinData(DataFlags, const Containers::ArrayView jointData, DataFlags, const Containers::ArrayView> inverseBindMatrixData, const void* const importerState) noexcept: SkinData{Containers::Array{const_cast(jointData.data()), jointData.size(), Implementation::nonOwnedArrayDeleter}, Containers::Array>{const_cast*>(inverseBindMatrixData.data()), inverseBindMatrixData.size(), Implementation::nonOwnedArrayDeleter}, importerState} {} template SkinData::SkinData(SkinData&&) noexcept = default; diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index ad0df45df..cf59e7000 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -3739,7 +3739,7 @@ void AbstractImporterTest::animationNonOwningDeleters() { Containers::Optional doAnimation(UnsignedInt) override { return AnimationData{Containers::Array{data, 1, Implementation::nonOwnedArrayDeleter}, Containers::Array{&track, 1, - reinterpret_cast(Implementation::nonOwnedArrayDeleter)}}; + Implementation::nonOwnedArrayDeleter}}; } char data[1];