From 036fced7498e7edc096b2d8c119023e811f0f26a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 6 Dec 2019 19:17:47 +0100 Subject: [PATCH] Trade: whitelist (exported) growable array deleters in imported data. --- src/Magnum/Trade/AbstractImporter.cpp | 13 +- src/Magnum/Trade/AbstractImporter.h | 7 +- src/Magnum/Trade/ArrayAllocator.cpp | 34 ++++++ src/Magnum/Trade/ArrayAllocator.h | 60 +++++++++ src/Magnum/Trade/CMakeLists.txt | 2 + .../Trade/Test/AbstractImporterTest.cpp | 114 ++++++++++++++++++ 6 files changed, 221 insertions(+), 9 deletions(-) create mode 100644 src/Magnum/Trade/ArrayAllocator.cpp create mode 100644 src/Magnum/Trade/ArrayAllocator.h diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index 50882268d..512794d6b 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -35,6 +35,7 @@ #include "Magnum/FileCallback.h" #include "Magnum/Trade/AbstractMaterialData.h" #include "Magnum/Trade/AnimationData.h" +#include "Magnum/Trade/ArrayAllocator.h" #include "Magnum/Trade/CameraData.h" #include "Magnum/Trade/ImageData.h" #include "Magnum/Trade/LightData.h" @@ -269,7 +270,7 @@ 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() || animation->_data.deleter() == Implementation::nonOwnedArrayDeleter || animation->_data.deleter() == ArrayAllocator::deleter) && (!animation->_tracks.deleter() || animation->_tracks.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), "Trade::AbstractImporter::animation(): implementation is not allowed to use a custom Array deleter", {}); return animation; @@ -434,8 +435,8 @@ Containers::Optional AbstractImporter::mesh(const UnsignedInt id) { CORRADE_ASSERT(id < doMeshCount(), "Trade::AbstractImporter::mesh(): index" << id << "out of range for" << doMeshCount() << "entries", {}); Containers::Optional mesh = doMesh(id); CORRADE_ASSERT(!mesh || ( - (!mesh->_indexData.deleter() || mesh->_indexData.deleter() == Implementation::nonOwnedArrayDeleter) && - (!mesh->_vertexData.deleter() || mesh->_vertexData.deleter() == Implementation::nonOwnedArrayDeleter) && + (!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))), "Trade::AbstractImporter::mesh(): implementation is not allowed to use a custom Array deleter", {}); return mesh; @@ -640,7 +641,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, "Trade::AbstractImporter::image1D(): implementation is not allowed to use a custom Array deleter", {}); + 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", {}); return image; } @@ -696,7 +697,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, "Trade::AbstractImporter::image2D(): implementation is not allowed to use a custom Array deleter", {}); + 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", {}); return image; } @@ -752,7 +753,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, "Trade::AbstractImporter::image3D(): implementation is not allowed to use a custom Array deleter", {}); + 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", {}); return image; } diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index 212c73575..bcc337430 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -255,9 +255,10 @@ checked by the implementation: As @ref Trade-AbstractImporter-data-dependency "mentioned above", @ref Corrade::Containers::Array instances returned from plugin implementations are not allowed to use anything else than the default - deleter, otherwise this could cause dangling function pointer call on array - destruction if the plugin gets unloaded before the array is destroyed. This - is asserted by the base implementation on return. + deleter or the deleter used by @ref Trade::ArrayAllocator, otherwise this + could cause dangling function pointer call on array destruction if the + plugin gets unloaded before the array is destroyed. This is asserted by the + base implementation on return. @par Similarly for interpolator functions passed through @ref Animation::TrackView instances to @ref AnimationData --- to avoid diff --git a/src/Magnum/Trade/ArrayAllocator.cpp b/src/Magnum/Trade/ArrayAllocator.cpp new file mode 100644 index 000000000..cc574e884 --- /dev/null +++ b/src/Magnum/Trade/ArrayAllocator.cpp @@ -0,0 +1,34 @@ +/* + This file is part of Magnum. + + Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 + Vladimír Vondruš + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included + in all copies or substantial portions of the Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. +*/ + +#include "ArrayAllocator.h" + +namespace Magnum { namespace Trade { + +void ArrayAllocator::deleter(char* const data, std::size_t) { + deallocate(data); +} + +}} diff --git a/src/Magnum/Trade/ArrayAllocator.h b/src/Magnum/Trade/ArrayAllocator.h new file mode 100644 index 000000000..29426effc --- /dev/null +++ b/src/Magnum/Trade/ArrayAllocator.h @@ -0,0 +1,60 @@ +#ifndef Magnum_Trade_ArrayAllocator_h +#define Magnum_Trade_ArrayAllocator_h +/* + This file is part of Magnum. + + Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 + Vladimír Vondruš + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included + in all copies or substantial portions of the Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. +*/ + +/** @file + * @brief Class @ref Magnum::Trade::ArrayAllocator + * @m_since_latest + */ + +#include + +#include "Magnum/Magnum.h" +#include "Magnum/Trade/visibility.h" + +namespace Magnum { namespace Trade { + +/** +@brief Growable array allocator to be used in importer plugins +@m_since_latest + +Compared to @ref Corrade::Containers::ArrayMallocAllocator ensures that the +@ref Array deleter function pointer is defined in the @ref Trade library and +not in the plugin binary itself, avoiding dangling function pointer call when +the data array is destructed after the plugin has been unloaded. Other than +that the behavior is identical. +*/ +template struct ArrayAllocator: Containers::ArrayMallocAllocator {}; + +#ifndef DOXYGEN_GENERATING_OUTPUT +template<> struct ArrayAllocator: Containers::ArrayMallocAllocator { + MAGNUM_TRADE_EXPORT static void deleter(char* data, std::size_t size); +}; +#endif + +}} + +#endif diff --git a/src/Magnum/Trade/CMakeLists.txt b/src/Magnum/Trade/CMakeLists.txt index 28afc0091..1b07fc5e1 100644 --- a/src/Magnum/Trade/CMakeLists.txt +++ b/src/Magnum/Trade/CMakeLists.txt @@ -27,6 +27,7 @@ find_package(Corrade REQUIRED PluginManager) set(MagnumTrade_SRCS AbstractMaterialData.cpp + ArrayAllocator.cpp Data.cpp LightData.cpp MeshData2D.cpp @@ -52,6 +53,7 @@ set(MagnumTrade_HEADERS AbstractImageConverter.h AbstractMaterialData.h AnimationData.h + ArrayAllocator.h CameraData.h Data.h ImageData.h diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index f3b97bade..9dc4256b6 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -34,6 +34,7 @@ #include "Magnum/FileCallback.h" #include "Magnum/Trade/AbstractImporter.h" #include "Magnum/Trade/AnimationData.h" +#include "Magnum/Trade/ArrayAllocator.h" #include "Magnum/Trade/CameraData.h" #include "Magnum/Trade/ImageData.h" #include "Magnum/Trade/LightData.h" @@ -107,6 +108,7 @@ struct AbstractImporterTest: TestSuite::Tester { void animationNoFile(); void animationOutOfRange(); void animationNonOwningDeleters(); + void animationGrowableDeleters(); void animationCustomDataDeleter(); void animationCustomTrackDeleter(); @@ -170,6 +172,7 @@ struct AbstractImporterTest: TestSuite::Tester { void meshNoFile(); void meshOutOfRange(); void meshNonOwningDeleters(); + void meshGrowableDeleters(); void meshCustomIndexDataDeleter(); void meshCustomVertexDataDeleter(); void meshCustomAttributesDeleter(); @@ -243,6 +246,7 @@ struct AbstractImporterTest: TestSuite::Tester { void image1DOutOfRange(); void image1DLevelOutOfRange(); void image1DNonOwningDeleter(); + void image1DGrowableDeleter(); void image1DCustomDeleter(); void image2D(); @@ -262,6 +266,7 @@ struct AbstractImporterTest: TestSuite::Tester { void image2DOutOfRange(); void image2DLevelOutOfRange(); void image2DNonOwningDeleter(); + void image2DGrowableDeleter(); void image2DCustomDeleter(); void image3D(); @@ -281,6 +286,7 @@ struct AbstractImporterTest: TestSuite::Tester { void image3DOutOfRange(); void image3DLevelOutOfRange(); void image3DNonOwningDeleter(); + void image3DGrowableDeleter(); void image3DCustomDeleter(); void importerState(); @@ -346,6 +352,7 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::animationNoFile, &AbstractImporterTest::animationOutOfRange, &AbstractImporterTest::animationNonOwningDeleters, + &AbstractImporterTest::animationGrowableDeleters, &AbstractImporterTest::animationCustomDataDeleter, &AbstractImporterTest::animationCustomTrackDeleter, @@ -409,6 +416,7 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::meshNoFile, &AbstractImporterTest::meshOutOfRange, &AbstractImporterTest::meshNonOwningDeleters, + &AbstractImporterTest::meshGrowableDeleters, &AbstractImporterTest::meshCustomIndexDataDeleter, &AbstractImporterTest::meshCustomVertexDataDeleter, &AbstractImporterTest::meshCustomAttributesDeleter, @@ -482,6 +490,7 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::image1DOutOfRange, &AbstractImporterTest::image1DLevelOutOfRange, &AbstractImporterTest::image1DNonOwningDeleter, + &AbstractImporterTest::image1DGrowableDeleter, &AbstractImporterTest::image1DCustomDeleter, &AbstractImporterTest::image2D, @@ -501,6 +510,7 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::image2DOutOfRange, &AbstractImporterTest::image2DLevelOutOfRange, &AbstractImporterTest::image2DNonOwningDeleter, + &AbstractImporterTest::image2DGrowableDeleter, &AbstractImporterTest::image2DCustomDeleter, &AbstractImporterTest::image3D, @@ -520,6 +530,7 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::image3DOutOfRange, &AbstractImporterTest::image3DLevelOutOfRange, &AbstractImporterTest::image3DNonOwningDeleter, + &AbstractImporterTest::image3DGrowableDeleter, &AbstractImporterTest::image3DCustomDeleter, &AbstractImporterTest::importerState, @@ -1405,6 +1416,25 @@ void AbstractImporterTest::animationNonOwningDeleters() { CORRADE_COMPARE(static_cast(data->data()), importer.data); } +void AbstractImporterTest::animationGrowableDeleters() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doAnimationCount() const override { return 1; } + Containers::Optional doAnimation(UnsignedInt) override { + Containers::Array data; + Containers::arrayAppend(data, '\x37'); + return AnimationData{std::move(data), {AnimationTrackData{}}}; + } + } importer; + + auto data = importer.animation(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(data->data()[0], '\x37'); +} + void AbstractImporterTest::animationCustomDataDeleter() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } @@ -2300,6 +2330,33 @@ void AbstractImporterTest::meshNonOwningDeleters() { CORRADE_COMPARE(static_cast(data->indexData()), importer.indexData); } +void AbstractImporterTest::meshGrowableDeleters() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doMeshCount() const override { return 1; } + Containers::Optional doMesh(UnsignedInt) override { + Containers::Array indexData; + Containers::arrayAppend(indexData, '\xab'); + Containers::Array vertexData; + Containers::arrayAppend(vertexData, Vector3{}); + MeshIndexData indices{MeshIndexType::UnsignedByte, indexData}; + MeshAttributeData positions{MeshAttribute::Position, Containers::arrayView(vertexData)}; + + return MeshData{MeshPrimitive::Triangles, + std::move(indexData), indices, + Containers::arrayAllocatorCast(std::move(vertexData)), {positions}}; + } + } importer; + + auto data = importer.mesh(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(data->indexData()[0], '\xab'); + CORRADE_COMPARE(data->vertexData().size(), 12); +} + void AbstractImporterTest::meshCustomIndexDataDeleter() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } @@ -3344,6 +3401,25 @@ void AbstractImporterTest::image1DNonOwningDeleter() { CORRADE_COMPARE(static_cast(data->data()), importer.data); } +void AbstractImporterTest::image1DGrowableDeleter() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doImage1DCount() const override { return 1; } + Containers::Optional doImage1D(UnsignedInt, UnsignedInt) override { + Containers::Array data; + Containers::arrayAppend(data, '\xff'); + return ImageData1D{PixelFormat::RGBA8Unorm, {}, std::move(data)}; + } + } importer; + + auto data = importer.image1D(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(data->data()[0], '\xff'); +} + void AbstractImporterTest::image1DCustomDeleter() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } @@ -3630,6 +3706,25 @@ void AbstractImporterTest::image2DNonOwningDeleter() { CORRADE_COMPARE(static_cast(data->data()), importer.data); } +void AbstractImporterTest::image2DGrowableDeleter() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doImage2DCount() const override { return 1; } + Containers::Optional doImage2D(UnsignedInt, UnsignedInt) override { + Containers::Array data; + Containers::arrayAppend(data, '\xff'); + return ImageData2D{PixelFormat::RGBA8Unorm, {}, std::move(data)}; + } + } importer; + + auto data = importer.image2D(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(data->data()[0], '\xff'); +} + void AbstractImporterTest::image2DCustomDeleter() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } @@ -3917,6 +4012,25 @@ void AbstractImporterTest::image3DNonOwningDeleter() { CORRADE_COMPARE(static_cast(data->data()), importer.data); } +void AbstractImporterTest::image3DGrowableDeleter() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doImage3DCount() const override { return 1; } + Containers::Optional doImage3D(UnsignedInt, UnsignedInt) override { + Containers::Array data; + Containers::arrayAppend(data, '\xff'); + return ImageData3D{PixelFormat::RGBA8Unorm, {}, std::move(data)}; + } + } importer; + + auto data = importer.image3D(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(data->data()[0], '\xff'); +} + void AbstractImporterTest::image3DCustomDeleter() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; }