From d0aca29ba52b44c49e22e30199fcd8c346b7c04f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 27 Mar 2023 15:00:30 +0200 Subject: [PATCH] Trade: rewrite MeshData bound assertions similarly to SceneData. There it's done in a significantly more robust way, without relying on `begin < end` but rather a negative stride. --- src/Magnum/Trade/MeshData.cpp | 69 ++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 96b995944..0695b8856 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -138,18 +138,28 @@ MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& inde const UnsignedInt typeSize = isMeshIndexTypeImplementationSpecific(_indexType) ? 0 : meshIndexTypeSize(_indexType); - - const void* begin = _indices; - /* C integer promotion rules are weird, without the Int the result is - an unsigned 32-bit value that messes things up on 64bit */ - const void* end = _indices + Int(_indexCount - 1)*_indexStride; - /* Flip for negative stride */ - if(begin > end) std::swap(begin, end); - /* Add the last element size to the higher address */ - end = static_cast(end) + typeSize; - - CORRADE_ASSERT(begin >= _indexData.begin() && end <= _indexData.end(), - "Trade::MeshData: indices [" << Debug::nospace << begin << Debug::nospace << ":" << Debug::nospace << end << Debug::nospace << "] are not contained in passed indexData array [" << Debug::nospace << static_cast(_indexData.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_indexData.end()) << Debug::nospace << "]", ); + /* For negative strides the size is negative. C integer promotion rules + are weird, without the Int the result is an unsigned 32-bit value + that messes things up on 64bit. */ + const std::ptrdiff_t signedSize = Int(_indexCount - 1)*_indexStride; + + /* Calculate begin and end offset. Both pointer and offset-only rely on + basically same calculation, do it with offsets in a single place and + only interpret as pointers in the non-offset-only check. */ + std::ptrdiff_t begin = 0; + std::ptrdiff_t end = typeSize; + /* For negative strides the begin pointer is moved backwards */ + if(_indexStride < 0) + begin += signedSize; + else + end += signedSize; + /* Add the base data offset to both begin and end. Yes, yes, this may + read the `pointer` union member through `offset`. */ + begin += reinterpret_cast(_indices); + end += reinterpret_cast(_indices); + + CORRADE_ASSERT(reinterpret_cast(begin) >= _indexData.begin() && reinterpret_cast(end) <= _indexData.end(), + "Trade::MeshData: indices [" << Debug::nospace << reinterpret_cast(begin) << Debug::nospace << ":" << Debug::nospace << reinterpret_cast(end) << Debug::nospace << "] are not contained in passed indexData array [" << Debug::nospace << static_cast(_indexData.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_indexData.end()) << Debug::nospace << "]", ); } /* Not checking what's already checked in MeshIndexData / MeshAttributeData @@ -171,22 +181,29 @@ MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& inde isVertexFormatImplementationSpecific(attribute._format) ? 0 : (vertexFormatSize(attribute._format)* (attribute._arraySize ? attribute._arraySize : 1)); - - /* Both pointer and offset-only rely on basically same calculation, - do it with offsets in a single place and only interpret as - pointers in the non-offset-only check. Yes, yes, this may read - the `pointer` union member through `offset`. */ - std::size_t begin = attribute._data.offset; - /* C integer promotion rules are weird, without the Int the result - is an unsigned 32-bit value that messes things up on 64bit */ - std::size_t end = begin + Int(_vertexCount - 1)*attribute._stride; - /* Flip for negative stride */ - if(begin > end) std::swap(begin, end); - /* Add the last element size to the higher address */ - end += typeSize; + /* For negative strides the size is negative. C integer promotion + rules are weird, without the Int the result is an unsigned + 32-bit value that messes things up on 64bit. */ + const std::ptrdiff_t signedSize = Int(_vertexCount - 1)*attribute._stride; + + /* Calculate begin and end offset. Both pointer and offset-only + rely on basically same calculation, do it with offsets in a + single place and only interpret as pointers in the + non-offset-only check. */ + std::ptrdiff_t begin = 0; + std::ptrdiff_t end = typeSize; + /* For negative strides the begin pointer is moved backwards */ + if(attribute._stride < 0) + begin += signedSize; + else + end += signedSize; + /* Add the base data offset to both begin and end. Yes, yes, this + may read the `pointer` union member through `offset`. */ + begin += attribute._data.offset; + end += attribute._data.offset; if(attribute._isOffsetOnly) { - CORRADE_ASSERT(end <= _vertexData.size(), + CORRADE_ASSERT(std::size_t(end) <= _vertexData.size(), "Trade::MeshData: offset-only attribute" << i << "spans" << end << "bytes but passed vertexData array has only" << _vertexData.size(), ); } else { CORRADE_ASSERT(reinterpret_cast(begin) >= _vertexData.begin() && reinterpret_cast(end) <= _vertexData.end(),