From bf6caece50900cdf45f7af8c9371da8a17a78fb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 30 Aug 2021 18:45:26 +0200 Subject: [PATCH] Trade: fix MeshData range checks for array attributes. And add a comment explaining why we don't check the pointer for empty meshes -- otherwise empty interleaved meshes would fail with stuff like Trade::MeshData: attribute 0 [0xc:0xc] is not contained in passed vertexData array [0x0:0x0] which ... helps nobody. --- doc/changelog.dox | 3 +++ src/Magnum/Trade/MeshData.cpp | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index e93873c5f..e56f9ca9f 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -472,6 +472,9 @@ See also: - @ref Trade::MeshData::attributeData(UnsignedInt) const was not correctly propagating attribute array size, causing array attributes to appear as non-array +- @ref Trade::MeshData constructor data view range assertions weren't + correctly taking sizes of array attributes into account, leading to meshes + with out-of-range attributes getting silently accepted - Fixed @ref Platform::GlfwApplication, @ref Platform::Sdl2Application and @ref Platform::EmscriptenApplication to correctly print app-specified DPI scaling in its verbose output diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 919ce9ced..4672f10a5 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -136,14 +136,23 @@ MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& inde check at least partially. */ const UnsignedInt typeSize = isVertexFormatImplementationSpecific(attribute._format) ? 0 : - vertexFormatSize(attribute._format); + (vertexFormatSize(attribute._format)* + (attribute._arraySize ? attribute._arraySize : 1)); if(attribute._isOffsetOnly) { const std::size_t size = attribute._data.offset + (_vertexCount - 1)*attribute._stride + typeSize; + /* For meshes with zero vertices we don't check the pointer, since + accessing the memory is invalid anyway and enforcing this would + lead to unnecessary friction with interleaved attributes of + empty meshes. */ CORRADE_ASSERT(!_vertexCount || size <= _vertexData.size(), "Trade::MeshData: offset-only attribute" << i << "spans" << size << "bytes but passed vertexData array has only" << _vertexData.size(), ); } else { const void* const begin = static_cast(attribute._data.pointer); const void* const end = static_cast(attribute._data.pointer) + (_vertexCount - 1)*attribute._stride + typeSize; + /* For meshes with zero vertices we don't check the pointer, since + accessing the memory is invalid anyway and enforcing this would + lead to unnecessary friction with interleaved attributes of + empty meshes. */ CORRADE_ASSERT(!_vertexCount || (begin >= _vertexData.begin() && end <= _vertexData.end()), "Trade::MeshData: attribute" << i << "[" << Debug::nospace << begin << Debug::nospace << ":" << Debug::nospace << end << Debug::nospace << "] is not contained in passed vertexData array [" << Debug::nospace << static_cast(_vertexData.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_vertexData.end()) << Debug::nospace << "]", ); }