From 5217f4849991063d1ff9f431341976d54dd7e531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 22 Aug 2020 17:26:39 +0200 Subject: [PATCH] Trade: disallow custom deleters in AbstractImporter::skinXD(). --- src/Magnum/Trade/AbstractImporter.cpp | 14 +- src/Magnum/Trade/AbstractImporter.h | 10 +- src/Magnum/Trade/SkinData.cpp | 3 +- src/Magnum/Trade/SkinData.h | 5 + .../Trade/Test/AbstractImporterTest.cpp | 184 +++++++++++++++++- 5 files changed, 206 insertions(+), 10 deletions(-) diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index 9b566a806..28dae34b1 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -501,7 +501,12 @@ std::string AbstractImporter::doSkin2DName(UnsignedInt) { return {}; } Containers::Optional AbstractImporter::skin2D(const UnsignedInt id) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::skin2D(): no file opened", {}); CORRADE_ASSERT(id < doSkin2DCount(), "Trade::AbstractImporter::skin2D(): index" << id << "out of range for" << doSkin2DCount() << "entries", {}); - return doSkin2D(id); + 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))), + "Trade::AbstractImporter::skin2D(): implementation is not allowed to use a custom Array deleter", {}); + return skin; } Containers::Optional AbstractImporter::doSkin2D(UnsignedInt) { @@ -543,7 +548,12 @@ std::string AbstractImporter::doSkin3DName(UnsignedInt) { return {}; } Containers::Optional AbstractImporter::skin3D(const UnsignedInt id) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::skin3D(): no file opened", {}); CORRADE_ASSERT(id < doSkin3DCount(), "Trade::AbstractImporter::skin3D(): index" << id << "out of range for" << doSkin3DCount() << "entries", {}); - return doSkin3D(id); + 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))), + "Trade::AbstractImporter::skin3D(): implementation is not allowed to use a custom Array deleter", {}); + return skin; } Containers::Optional AbstractImporter::doSkin3D(UnsignedInt) { diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index 5fcb25934..6e572cb2d 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -245,11 +245,11 @@ dependency on the importer instance and neither on the dynamic plugin module. In other words, you don't need to keep the importer instance (or the plugin manager instance) around in order to have the `*Data` instances valid. Moreover, all @ref Corrade::Containers::Array instances returned through -@ref ImageData, @ref AnimationData, @ref MaterialData and @ref MeshData are -only allowed to have default deleters (or be non-owning instances created from -@ref Corrade::Containers::ArrayView) --- this is to avoid potential dangling -function pointer calls when destructing such instances after the plugin module -has been unloaded. +@ref ImageData, @ref AnimationData, @ref MaterialData, @ref MeshData and +@ref SkinData are only allowed to have default deleters (or be non-owning +instances created from @ref Corrade::Containers::ArrayView) --- this is to +avoid potential dangling function pointer calls when destructing such instances +after the plugin module has been unloaded. The only exception are various `importerState()` functions @ref Trade-AbstractImporter-usage-state "described above", but in that case the diff --git a/src/Magnum/Trade/SkinData.cpp b/src/Magnum/Trade/SkinData.cpp index 34d722f58..4d42d19fc 100644 --- a/src/Magnum/Trade/SkinData.cpp +++ b/src/Magnum/Trade/SkinData.cpp @@ -28,6 +28,7 @@ #include "Magnum/Math/Matrix3.h" #include "Magnum/Math/Matrix4.h" #include "Magnum/Trade/Data.h" +#include "Magnum/Trade/Implementation/arrayUtilities.h" namespace Magnum { namespace Trade { @@ -36,7 +37,7 @@ template SkinData::SkinData(Containers::Arra "Trade::SkinData: joint and inverse bind matrix arrays have different size, got" << _jointData.size() << "and" << _inverseBindMatrixData.size(), ); } -template SkinData::SkinData(const std::initializer_list joints, const std::initializer_list> inverseBindMatrices, const void* const importerState): SkinData{Containers::array(joints), Containers::array(inverseBindMatrices), importerState} {} +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} {} diff --git a/src/Magnum/Trade/SkinData.h b/src/Magnum/Trade/SkinData.h index 232e6d235..42b2ab778 100644 --- a/src/Magnum/Trade/SkinData.h +++ b/src/Magnum/Trade/SkinData.h @@ -127,6 +127,11 @@ template class SkinData { const void* importerState() const { return _importerState; } private: + /* For custom deleter checks. Not done in the constructors here because + the restriction is pointless when used outside of plugin + implementations. */ + friend AbstractImporter; + /** @todo skeleton object ID? gltf has that but the use is unclear */ Containers::Array _jointData; Containers::Array> _inverseBindMatrixData; diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index 45a4c564b..84a755b08 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -147,12 +147,18 @@ struct AbstractImporterTest: TestSuite::Tester { void skin2DNameOutOfRange(); void skin2DNotImplemented(); void skin2DOutOfRange(); + void skin2DNonOwningDeleters(); + void skin2DCustomJointDataDeleter(); + void skin2DCustomInverseBindMatrixDataDeleter(); void skin3D(); void skin3DNameNotImplemented(); void skin3DNameOutOfRange(); void skin3DNotImplemented(); void skin3DOutOfRange(); + void skin3DNonOwningDeleters(); + void skin3DCustomJointDataDeleter(); + void skin3DCustomInverseBindMatrixDataDeleter(); void mesh(); #ifdef MAGNUM_BUILD_DEPRECATED @@ -367,12 +373,18 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::skin2DNameOutOfRange, &AbstractImporterTest::skin2DNotImplemented, &AbstractImporterTest::skin2DOutOfRange, + &AbstractImporterTest::skin2DNonOwningDeleters, + &AbstractImporterTest::skin2DCustomJointDataDeleter, + &AbstractImporterTest::skin2DCustomInverseBindMatrixDataDeleter, &AbstractImporterTest::skin3D, &AbstractImporterTest::skin3DNameNotImplemented, &AbstractImporterTest::skin3DNameOutOfRange, &AbstractImporterTest::skin3DNotImplemented, &AbstractImporterTest::skin3DOutOfRange, + &AbstractImporterTest::skin3DNonOwningDeleters, + &AbstractImporterTest::skin3DCustomJointDataDeleter, + &AbstractImporterTest::skin3DCustomInverseBindMatrixDataDeleter, &AbstractImporterTest::mesh, #ifdef MAGNUM_BUILD_DEPRECATED @@ -2201,7 +2213,9 @@ void AbstractImporterTest::skin2D() { return {}; } Containers::Optional doSkin2D(UnsignedInt id) override { - if(id == 7) return SkinData2D{{}, {}, &state}; + /* Verify that initializer list is converted to an array with + the default deleter and not something disallowed */ + if(id == 7) return SkinData2D{{1}, {{}}, &state}; return {}; } } importer; @@ -2293,6 +2307,88 @@ void AbstractImporterTest::skin2DOutOfRange() { CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::skin2D(): index 8 out of range for 8 entries\n"); } +void AbstractImporterTest::skin2DNonOwningDeleters() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSkin2DCount() const override { return 1; } + Int doSkin2DForName(const std::string&) override { return 0; } + Containers::Optional doSkin2D(UnsignedInt) override { + return SkinData2D{{}, jointData, {}, inverseBindMatrixData}; + } + + UnsignedInt jointData[1]{}; + Matrix3 inverseBindMatrixData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + auto data = importer.skin2D(0); + CORRADE_COMPARE(data->joints(), importer.jointData); + CORRADE_COMPARE(data->inverseBindMatrices(), importer.inverseBindMatrixData); +} + +void AbstractImporterTest::skin2DCustomJointDataDeleter() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSkin2DCount() const override { return 1; } + Int doSkin2DForName(const std::string&) override { return 0; } + Containers::Optional doSkin2D(UnsignedInt) override { + return SkinData2D{Containers::Array{jointData, 1, [](UnsignedInt*, std::size_t){}}, Containers::Array{1}}; + } + + UnsignedInt jointData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + importer.skin2D(0); + importer.skin2D(""); + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::skin2D(): implementation is not allowed to use a custom Array deleter\n" + "Trade::AbstractImporter::skin2D(): implementation is not allowed to use a custom Array deleter\n"); +} + +void AbstractImporterTest::skin2DCustomInverseBindMatrixDataDeleter() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSkin2DCount() const override { return 1; } + Int doSkin2DForName(const std::string&) override { return 0; } + Containers::Optional doSkin2D(UnsignedInt) override { + return SkinData2D{Containers::Array{1}, Containers::Array{inverseBindMatrixData, 1, [](Matrix3*, std::size_t){}}}; + } + + Matrix3 inverseBindMatrixData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + importer.skin2D(0); + importer.skin2D(""); + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::skin2D(): implementation is not allowed to use a custom Array deleter\n" + "Trade::AbstractImporter::skin2D(): implementation is not allowed to use a custom Array deleter\n"); +} + void AbstractImporterTest::skin3D() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } @@ -2309,7 +2405,9 @@ void AbstractImporterTest::skin3D() { return {}; } Containers::Optional doSkin3D(UnsignedInt id) override { - if(id == 7) return SkinData3D{{}, {}, &state}; + /* Verify that initializer list is converted to an array with + the default deleter and not something disallowed */ + if(id == 7) return SkinData3D{{1}, {{}}, &state}; return {}; } } importer; @@ -2401,6 +2499,88 @@ void AbstractImporterTest::skin3DOutOfRange() { CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::skin3D(): index 8 out of range for 8 entries\n"); } +void AbstractImporterTest::skin3DNonOwningDeleters() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSkin3DCount() const override { return 1; } + Int doSkin3DForName(const std::string&) override { return 0; } + Containers::Optional doSkin3D(UnsignedInt) override { + return SkinData3D{{}, jointData, {}, inverseBindMatrixData}; + } + + UnsignedInt jointData[1]{}; + Matrix4 inverseBindMatrixData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + auto data = importer.skin3D(0); + CORRADE_COMPARE(data->joints(), importer.jointData); + CORRADE_COMPARE(data->inverseBindMatrices(), importer.inverseBindMatrixData); +} + +void AbstractImporterTest::skin3DCustomJointDataDeleter() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSkin3DCount() const override { return 1; } + Int doSkin3DForName(const std::string&) override { return 0; } + Containers::Optional doSkin3D(UnsignedInt) override { + return SkinData3D{Containers::Array{jointData, 1, [](UnsignedInt*, std::size_t){}}, Containers::Array{1}}; + } + + UnsignedInt jointData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + importer.skin3D(0); + importer.skin3D(""); + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::skin3D(): implementation is not allowed to use a custom Array deleter\n" + "Trade::AbstractImporter::skin3D(): implementation is not allowed to use a custom Array deleter\n"); +} + +void AbstractImporterTest::skin3DCustomInverseBindMatrixDataDeleter() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSkin3DCount() const override { return 1; } + Int doSkin3DForName(const std::string&) override { return 0; } + Containers::Optional doSkin3D(UnsignedInt) override { + return SkinData3D{Containers::Array{1}, Containers::Array{inverseBindMatrixData, 1, [](Matrix4*, std::size_t){}}}; + } + + Matrix4 inverseBindMatrixData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + importer.skin3D(0); + importer.skin3D(""); + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::skin3D(): implementation is not allowed to use a custom Array deleter\n" + "Trade::AbstractImporter::skin3D(): implementation is not allowed to use a custom Array deleter\n"); +} + void AbstractImporterTest::mesh() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; }