From 6d8999eefdf37c89079c29e99aa6ab43c0a09bc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 15 Feb 2023 22:10:02 +0100 Subject: [PATCH] Trade: expect MaterialData attrs to not be larger than end layer offset. Such an unnecessary footgun -- I was already checking the other case, having attribute data too short, but this I thought is fine because it's not leading to any crash. Well it's leading to needless pain and suffering, that's what it is doing! And of course already found FIVE such bugs in just Magnum tests alone. --- .../Test/PhongToPbrMetallicRoughnessTest.cpp | 4 +- src/Magnum/Trade/MaterialData.cpp | 6 +++ src/Magnum/Trade/MaterialData.h | 4 +- .../Trade/Test/AbstractImporterTest.cpp | 2 +- src/Magnum/Trade/Test/MaterialDataTest.cpp | 42 ++++++++++++++++++- .../Test/PbrClearCoatMaterialDataTest.cpp | 2 +- 6 files changed, 53 insertions(+), 7 deletions(-) diff --git a/src/Magnum/MaterialTools/Test/PhongToPbrMetallicRoughnessTest.cpp b/src/Magnum/MaterialTools/Test/PhongToPbrMetallicRoughnessTest.cpp index 400072c30..47be5af1d 100644 --- a/src/Magnum/MaterialTools/Test/PhongToPbrMetallicRoughnessTest.cpp +++ b/src/Magnum/MaterialTools/Test/PhongToPbrMetallicRoughnessTest.cpp @@ -152,7 +152,7 @@ const struct { {Trade::MaterialAttribute::AlphaMask, 0.7f}, {Trade::MaterialLayer::ClearCoat}, {Trade::MaterialAttribute::LayerFactor, 0.35f}, - }, {2, 4}}, + }, {3, 5}}, Trade::MaterialData{Trade::MaterialType::PbrMetallicRoughness, { {Trade::MaterialAttribute::BaseColor, 0xff3366ff_rgbaf}, {Trade::MaterialAttribute::BaseColorTexture, 0u}, @@ -161,7 +161,7 @@ const struct { {Trade::MaterialAttribute::AlphaMask, 0.7f}, {Trade::MaterialLayer::ClearCoat}, {Trade::MaterialAttribute::LayerFactor, 0.35f}, - }, {4, 6}}}, + }, {5, 7}}}, {"diffuse texture properties without texture", {}, Trade::MaterialData{{}, { {Trade::MaterialAttribute::DiffuseColor, 0xff3366cc_rgbaf}, diff --git a/src/Magnum/Trade/MaterialData.cpp b/src/Magnum/Trade/MaterialData.cpp index 68e839878..94b982bb1 100644 --- a/src/Magnum/Trade/MaterialData.cpp +++ b/src/Magnum/Trade/MaterialData.cpp @@ -321,6 +321,9 @@ 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} {} @@ -358,6 +361,9 @@ MaterialData::MaterialData(const MaterialTypes types, const DataFlags attributeD begin = end; } + + CORRADE_ASSERT(layerOffsets.back() == _data.size(), + "Trade::MaterialData: last layer offset" << layerOffsets.back() << "too short for" << _data.size() << "attributes in total", ); #endif } diff --git a/src/Magnum/Trade/MaterialData.h b/src/Magnum/Trade/MaterialData.h index 662c4db3a..f2b61c362 100644 --- a/src/Magnum/Trade/MaterialData.h +++ b/src/Magnum/Trade/MaterialData.h @@ -2095,7 +2095,7 @@ class MAGNUM_TRADE_EXPORT MaterialData { * The @p attributeData gets sorted by name internally, expecting no * duplicates inside each layer. The @p layerData is expected to be * either empty or a monotonically non-decreasing sequence of offsets - * not larger than @p attributeData size, with *i*-th item specifying + * counting up to @p attributeData size, with *i*-th item specifying * end offset of *i*-th layer. * * The @ref attributeDataFlags() / @ref layerDataFlags() are implicitly @@ -2123,7 +2123,7 @@ class MAGNUM_TRADE_EXPORT MaterialData { * The @p data is expected to be already sorted by name, without * duplicates inside each layer. The @p layerData is expected to be * either empty or a monotonically non-decreasing sequence of offsets - * not larger than @p attributeData size, with *i*-th item specifying + * counting up to @p attributeData size, with *i*-th item specifying * end offset of *i*-th layer. * * The @p attributeDataFlags / @p layerDataFlags parameters can contain diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index 23dd7fc25..e08414e73 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -6515,7 +6515,7 @@ void AbstractImporterTest::materialNonOwningDeleters() { return MaterialData{{}, {}, attributeData, {}, layerData}; } - UnsignedInt layerData[1]{}; + UnsignedInt layerData[1]{1}; MaterialAttributeData attributeData[1]{ {MaterialAttribute::DiffuseColor, Color4{}} }; diff --git a/src/Magnum/Trade/Test/MaterialDataTest.cpp b/src/Magnum/Trade/Test/MaterialDataTest.cpp index f73a53d3d..0124385c9 100644 --- a/src/Magnum/Trade/Test/MaterialDataTest.cpp +++ b/src/Magnum/Trade/Test/MaterialDataTest.cpp @@ -100,6 +100,7 @@ struct MaterialDataTest: TestSuite::Tester { void constructLayers(); void constructLayersNotMonotonic(); void constructLayersOffsetOutOfBounds(); + void constructLayersLastOffsetTooShort(); void constructNonOwned(); void constructNonOwnedLayers(); @@ -108,6 +109,7 @@ struct MaterialDataTest: TestSuite::Tester { void constructNonOwnedDuplicateAttribute(); void constructNonOwnedLayersNotMonotonic(); void constructNonOwnedLayersOffsetOutOfBounds(); + void constructNonOwnedLayersLastOffsetTooShort(); void constructNonOwnedAttributeFlagOwned(); void constructNonOwnedLayerFlagOwned(); @@ -267,6 +269,7 @@ MaterialDataTest::MaterialDataTest() { &MaterialDataTest::constructLayers, &MaterialDataTest::constructLayersNotMonotonic, &MaterialDataTest::constructLayersOffsetOutOfBounds, + &MaterialDataTest::constructLayersLastOffsetTooShort, &MaterialDataTest::constructNonOwned, &MaterialDataTest::constructNonOwnedLayers, @@ -275,6 +278,7 @@ MaterialDataTest::MaterialDataTest() { &MaterialDataTest::constructNonOwnedDuplicateAttribute, &MaterialDataTest::constructNonOwnedLayersNotMonotonic, &MaterialDataTest::constructNonOwnedLayersOffsetOutOfBounds, + &MaterialDataTest::constructNonOwnedLayersLastOffsetTooShort, &MaterialDataTest::constructNonOwnedAttributeFlagOwned, &MaterialDataTest::constructNonOwnedLayerFlagOwned, @@ -1511,6 +1515,20 @@ void MaterialDataTest::constructLayersOffsetOutOfBounds() { CORRADE_COMPARE(out.str(), "Trade::MaterialData: invalid range (2, 6) for layer 1 with 5 attributes in total\n"); } +void MaterialDataTest::constructLayersLastOffsetTooShort() { + CORRADE_SKIP_IF_NO_ASSERT(); + + std::ostringstream out; + Error redirectError{&out}; + MaterialData data{MaterialType::Phong, { + {MaterialAttribute::DoubleSided, true}, + {MaterialLayer::ClearCoat}, + {MaterialAttribute::LayerFactor, 0.5f}, + {MaterialAttribute::NormalTexture, 3u} + }, {1, 3}}; + CORRADE_COMPARE(out.str(), "Trade::MaterialData: last layer offset 3 too short for 4 attributes in total\n"); +} + void MaterialDataTest::constructNonOwned() { constexpr MaterialAttributeData attributes[]{ {"AmbientTextureMatrix"_s, Matrix3{{0.5f, 0.0f, 0.0f}, @@ -1700,6 +1718,28 @@ void MaterialDataTest::constructNonOwnedLayersOffsetOutOfBounds() { CORRADE_COMPARE(out.str(), "Trade::MaterialData: invalid range (2, 6) for layer 1 with 5 attributes in total\n"); } +void MaterialDataTest::constructNonOwnedLayersLastOffsetTooShort() { + CORRADE_SKIP_IF_NO_ASSERT(); + + MaterialAttributeData attributes[]{ + {MaterialAttribute::DoubleSided, true}, + {MaterialLayer::ClearCoat}, + {MaterialAttribute::LayerFactor, 0.5f}, + {MaterialAttribute::NormalTexture, 3u} + }; + + UnsignedInt layers[]{ + 1, 3 + }; + + std::ostringstream out; + Error redirectError{&out}; + MaterialData data{MaterialType::Phong, + {}, attributes, + {}, layers}; + CORRADE_COMPARE(out.str(), "Trade::MaterialData: last layer offset 3 too short for 4 attributes in total\n"); +} + void MaterialDataTest::constructNonOwnedAttributeFlagOwned() { CORRADE_SKIP_IF_NO_ASSERT(); @@ -2953,7 +2993,7 @@ void MaterialDataTest::accessNotFoundInLayerString() { MaterialData data{{}, { {MaterialAttribute::LayerName, "ClearCoat"}, {"DiffuseColor", 0xff3366aa_rgbaf} - }, {0, 1}}; + }, {0, 2}}; /* These are fine */ CORRADE_VERIFY(!data.hasAttribute("ClearCoat", "DiffuseColour")); diff --git a/src/Magnum/Trade/Test/PbrClearCoatMaterialDataTest.cpp b/src/Magnum/Trade/Test/PbrClearCoatMaterialDataTest.cpp index bdf8434d3..bf35ae2f5 100644 --- a/src/Magnum/Trade/Test/PbrClearCoatMaterialDataTest.cpp +++ b/src/Magnum/Trade/Test/PbrClearCoatMaterialDataTest.cpp @@ -464,7 +464,7 @@ void PbrClearCoatMaterialDataTest::commonCoordinatesLayerImplicit() { {textureName, 5u}, {textureName + "Coordinates", 0u}, {textureName + "Layer", 0u}, - }, {0, 3}}; + }, {0, 4}}; /* Zero is treated same as if there would be no attribute at all */ CORRADE_VERIFY(!data.hasTextureCoordinates());