From f5822e9384e78ab7458adc85725bae3beb2ff9e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 21 May 2020 13:40:34 +0200 Subject: [PATCH] Trade: properly include array size in attribute stride checks. --- src/Magnum/Trade/MeshData.cpp | 7 +++++-- src/Magnum/Trade/MeshData.h | 13 +++++++++---- src/Magnum/Trade/Test/MeshDataTest.cpp | 4 +++- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 01701d014..7c008215e 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -26,6 +26,9 @@ #include "MeshData.h" #include +#ifndef CORRADE_NO_ASSERT +#include +#endif #include "Magnum/Math/Color.h" #include "Magnum/Math/PackingBatch.h" @@ -62,8 +65,8 @@ MeshAttributeData::MeshAttributeData(const MeshAttribute name, const VertexForma because I feel that makes more sense than duplicating the full assert logic */ /** @todo support zero / negative stride? would be hard to transfer to GL */ - CORRADE_ASSERT(data.empty() || isVertexFormatImplementationSpecific(format) || std::ptrdiff_t(vertexFormatSize(format)) <= data.stride(), - "Trade::MeshAttributeData: expected stride to be positive and enough to fit" << format << Debug::nospace << ", got" << data.stride(), ); + CORRADE_ASSERT(data.empty() || isVertexFormatImplementationSpecific(format) || std::ptrdiff_t(vertexFormatSize(format))*(arraySize ? arraySize : 1) <= data.stride(), + "Trade::MeshAttributeData: expected stride to be positive and enough to fit" << format << Debug::nospace << (arraySize ? Utility::format("[{}]", arraySize).data() : "") << Debug::nospace << ", got" << data.stride(), ); } MeshAttributeData::MeshAttributeData(const MeshAttribute name, const VertexFormat format, UnsignedShort arraySize, const Containers::StridedArrayView2D& data) noexcept: MeshAttributeData{name, format, arraySize, Containers::StridedArrayView1D{{data.data(), ~std::size_t{}}, data.size()[0], data.stride()[0]}, nullptr} { diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index 072c50fa4..6695a1362 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -359,10 +359,10 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * @param arraySize Array size * @param data Attribute data * - * Expects that @p data stride is large enough to fit @p type, @p type - * corresponds to @p name and @p arraySize is zero for builtin - * attributes. Passing @cpp 0 @ce to @p arraySize is equivalent to - * calling the above overload. + * Expects that @p data stride is large enough to fit all @p arraySize + * items of @p type, @p type corresponds to @p name and @p arraySize is + * zero for builtin attributes. Passing @cpp 0 @ce to @p arraySize is + * equivalent to calling the above overload. */ explicit MeshAttributeData(MeshAttribute name, VertexFormat format, UnsignedShort arraySize, const Containers::StridedArrayView1D& data) noexcept; @@ -475,6 +475,11 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * Additionally, for even more flexibility, the @p vertexCount can be * overriden at @ref MeshData construction time, however all attributes * are still required to have the same vertex count to catch accidents. + * + * Note that due to the @cpp constexpr @ce nature of this constructor, + * no @p format / @p arraySize checks against @p stride can be done. + * You're encouraged to use the @ref MeshAttributeData(MeshAttribute, VertexFormat, const Containers::StridedArrayView1D&) + * constructor if you want additional safeguards. * @see @ref isOffsetOnly(), @ref arraySize(), * @ref data(Containers::ArrayView) const */ diff --git a/src/Magnum/Trade/Test/MeshDataTest.cpp b/src/Magnum/Trade/Test/MeshDataTest.cpp index b5531e859..8ae491ca8 100644 --- a/src/Magnum/Trade/Test/MeshDataTest.cpp +++ b/src/Magnum/Trade/Test/MeshDataTest.cpp @@ -750,6 +750,7 @@ void MeshDataTest::constructAttributeWrongStride() { std::ostringstream out; Error redirectError{&out}; MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector3, Containers::arrayCast(positionData)}; + MeshAttributeData{meshAttributeCustom(1), VertexFormat::Float, 4, Containers::arrayCast(positionData)}; /* We need this one to be constexpr, which means there can't be a warning about stride not matching the size */ MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector3, 0, 3*sizeof(Vector3), 1}; @@ -758,6 +759,7 @@ void MeshDataTest::constructAttributeWrongStride() { MeshAttributeData{65000}; CORRADE_COMPARE(out.str(), "Trade::MeshAttributeData: expected stride to be positive and enough to fit VertexFormat::Vector3, got 1\n" + "Trade::MeshAttributeData: expected stride to be positive and enough to fit VertexFormat::Float[4], got 12\n" "Trade::MeshAttributeData: expected stride to be positive and at most 32k, got -16\n" "Trade::MeshAttributeData: expected stride to be positive and at most 32k, got 65000\n" "Trade::MeshAttributeData: at most 32k padding supported, got 65000\n" @@ -931,7 +933,7 @@ void MeshDataTest::constructArrayAttributeNotAllowed() { /* This is not */ std::ostringstream out; Error redirectError{&out}; - MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 3, Containers::arrayView(positionData)}; + MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2b, 3, Containers::arrayView(positionData)}; MeshAttributeData{meshAttributeCustom(35), vertexFormatWrap(0xdead), 3, Containers::arrayView(positionData)}; MeshAttributeData{MeshAttribute::Position, positions2D}; MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 3, positions2Dchar};