diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index 6af41487f..f18f7ee2e 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -593,22 +593,32 @@ SceneData::SceneData(const SceneMappingType mappingType, const UnsignedLong mapp if(field._size) { const UnsignedInt fieldTypeSize = sceneFieldTypeSize(field._fieldType)* (field._fieldArraySize ? field._fieldArraySize : 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 mappingBegin = field._mappingData.offset; + std::size_t fieldBegin = field._fieldData.offset; + std::size_t mappingEnd = mappingBegin + (field._size - 1)*field._mappingStride; + std::size_t fieldEnd = fieldBegin + (field._size - 1)*field._fieldStride; + /* Flip for negative strides */ + if(mappingBegin > mappingEnd) std::swap(mappingBegin, mappingEnd); + if(fieldBegin > fieldEnd) std::swap(fieldBegin, fieldEnd); + /* Add the last element size to the higher address */ + mappingEnd += mappingTypeSize; + fieldEnd += fieldTypeSize; + if(field._flags & SceneFieldFlag::OffsetOnly) { - const std::size_t mappingSize = field._mappingData.offset + (field._size - 1)*field._mappingStride + mappingTypeSize; - const std::size_t fieldSize = field._fieldData.offset + (field._size - 1)*field._fieldStride + fieldTypeSize; - CORRADE_ASSERT(mappingSize <= _data.size(), - "Trade::SceneData: offset-only mapping data of field" << i << "span" << mappingSize << "bytes but passed data array has only" << _data.size(), ); - CORRADE_ASSERT(fieldSize <= _data.size(), - "Trade::SceneData: offset-only field data of field" << i << "span" << fieldSize << "bytes but passed data array has only" << _data.size(), ); + CORRADE_ASSERT(mappingEnd <= _data.size(), + "Trade::SceneData: offset-only mapping data of field" << i << "span" << mappingEnd << "bytes but passed data array has only" << _data.size(), ); + CORRADE_ASSERT(fieldEnd <= _data.size(), + "Trade::SceneData: offset-only field data of field" << i << "span" << fieldEnd << "bytes but passed data array has only" << _data.size(), ); } else { - const void* const mappingBegin = field._mappingData.pointer; - const void* const fieldBegin = field._fieldData.pointer; - const void* const mappingEnd = static_cast(field._mappingData.pointer) + (field._size - 1)*field._mappingStride + mappingTypeSize; - const void* const fieldEnd = static_cast(field._fieldData.pointer) + (field._size - 1)*field._fieldStride + fieldTypeSize; - CORRADE_ASSERT(mappingBegin >= _data.begin() && mappingEnd <= _data.end(), - "Trade::SceneData: mapping data [" << Debug::nospace << mappingBegin << Debug::nospace << ":" << Debug::nospace << mappingEnd << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); - CORRADE_ASSERT(fieldBegin >= _data.begin() && fieldEnd <= _data.end(), - "Trade::SceneData: field data [" << Debug::nospace << fieldBegin << Debug::nospace << ":" << Debug::nospace << fieldEnd << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); + CORRADE_ASSERT(reinterpret_cast(mappingBegin) >= _data.begin() && reinterpret_cast(mappingEnd) <= _data.end(), + "Trade::SceneData: mapping data [" << Debug::nospace << reinterpret_cast(mappingBegin) << Debug::nospace << ":" << Debug::nospace << reinterpret_cast(mappingEnd) << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); + CORRADE_ASSERT(reinterpret_cast(fieldBegin) >= _data.begin() && reinterpret_cast(fieldEnd) <= _data.end(), + "Trade::SceneData: field data [" << Debug::nospace << reinterpret_cast(fieldBegin) << Debug::nospace << ":" << Debug::nospace << reinterpret_cast(fieldEnd) << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); } } #endif diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index ff2f802c7..e7e3ba60a 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -1826,7 +1826,8 @@ void SceneDataTest::constructMappingDataNotContained() { CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); #endif - Containers::Array data{reinterpret_cast(0xbadda9), 10, [](char*, std::size_t){}}; + const Containers::Array data{reinterpret_cast(0xbadda9), 10, [](char*, std::size_t){}}; + Containers::Array sameDataButMovable{reinterpret_cast(0xbadda9), 10, [](char*, std::size_t){}}; Containers::ArrayView dataIn{reinterpret_cast(0xbadda9), 5}; Containers::ArrayView dataSlightlyOut{reinterpret_cast(0xbaddaa), 5}; Containers::ArrayView dataOut{reinterpret_cast(0xdead), 5}; @@ -1843,7 +1844,7 @@ void SceneDataTest::constructMappingDataNotContained() { SceneFieldData{SceneField::Mesh, dataOut, dataIn} }}; /* Verify the owning constructor does the checks as well */ - SceneData{SceneMappingType::UnsignedShort, 5, std::move(data), { + SceneData{SceneMappingType::UnsignedShort, 5, std::move(sameDataButMovable), { SceneFieldData{SceneField::Light, dataIn, dataIn}, SceneFieldData{SceneField::Mesh, dataOut, dataIn} }}; @@ -1852,16 +1853,27 @@ void SceneDataTest::constructMappingDataNotContained() { SceneData{SceneMappingType::UnsignedShort, 5, nullptr, { SceneFieldData{SceneField::Mesh, dataOut, dataIn} }}; - /* Finally, offset-only fields with a different message */ + /* Offset-only fields with a different message */ SceneData{SceneMappingType::UnsignedByte, 6, Containers::Array{24}, { SceneFieldData{SceneField::Mesh, 6, SceneMappingType::UnsignedByte, 4, 4, SceneFieldType::UnsignedByte, 0, 4} }}; + /* And the final boss, negative strides. Both only caught if the element + size gets properly added to the larger offset, not just the "end". */ + SceneData{SceneMappingType::UnsignedShort, 5, {}, data, { + SceneFieldData{SceneField::Mesh, stridedArrayView(dataSlightlyOut).flipped<0>(), dataIn} + }}; + SceneData{SceneMappingType::UnsignedByte, 6, Containers::Array{24}, { + SceneFieldData{SceneField::Mesh, 6, SceneMappingType::UnsignedByte, 24, -4, SceneFieldType::UnsignedByte, 0, 4} + }}; CORRADE_COMPARE(out.str(), "Trade::SceneData: mapping data [0xbaddaa:0xbaddb4] of field 0 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: mapping data [0xdead:0xdeb7] of field 1 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: mapping data [0xdead:0xdeb7] of field 1 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: mapping data [0xdead:0xdeb7] of field 0 are not contained in passed data array [0x0:0x0]\n" + "Trade::SceneData: offset-only mapping data of field 0 span 25 bytes but passed data array has only 24\n" + + "Trade::SceneData: mapping data [0xbaddaa:0xbaddb4] of field 0 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: offset-only mapping data of field 0 span 25 bytes but passed data array has only 24\n"); } @@ -1873,7 +1885,8 @@ void SceneDataTest::constructFieldDataNotContained() { /* Mostly the same as constructMappingDataNotContained() with mapping and field views swapped, and added checks for array fields */ - Containers::Array data{reinterpret_cast(0xbadda9), 10, [](char*, std::size_t){}}; + const Containers::Array data{reinterpret_cast(0xbadda9), 10, [](char*, std::size_t){}}; + Containers::Array sameDataButMovable{reinterpret_cast(0xbadda9), 10, [](char*, std::size_t){}}; Containers::ArrayView dataIn{reinterpret_cast(0xbadda9), 5}; Containers::ArrayView dataSlightlyOut{reinterpret_cast(0xbaddaa), 5}; Containers::ArrayView dataOut{reinterpret_cast(0xdead), 5}; @@ -1895,13 +1908,13 @@ void SceneDataTest::constructFieldDataNotContained() { SceneFieldData{sceneFieldCustom(37), dataIn.prefix(2), Containers::StridedArrayView2D{Containers::ArrayView{reinterpret_cast(0xbadda9), 12}, {2, 6}}} }}; /* Verify the owning constructor does the checks as well */ - SceneData{SceneMappingType::UnsignedShort, 5, std::move(data), { + SceneData{SceneMappingType::UnsignedShort, 5, std::move(sameDataButMovable), { SceneFieldData{SceneField::Light, dataIn, dataIn}, SceneFieldData{SceneField::Mesh, dataIn, dataOut} }}; /* Not checking for nullptr data, since that got checked for mapping view already and there's no way to trigger it for fields */ - /* Finally, offset-only fields with a different message */ + /* Offset-only fields with a different message */ SceneData{SceneMappingType::UnsignedShort, 6, Containers::Array{24}, { SceneFieldData{SceneField::Mesh, 6, SceneMappingType::UnsignedShort, 0, 4, SceneFieldType::UnsignedByte, 4, 4} }}; @@ -1910,6 +1923,14 @@ void SceneDataTest::constructFieldDataNotContained() { SceneData{SceneMappingType::UnsignedShort, 5, Containers::Array{24}, { SceneFieldData{sceneFieldCustom(37), 5, SceneMappingType::UnsignedShort, 0, 5, SceneFieldType::UnsignedByte, 0, 5, 5} }}; + /* And the final boss, negative strides. Both only caught if the element + size gets properly added to the larger offset, not just the "end". */ + SceneData{SceneMappingType::UnsignedShort, 5, {}, data, { + SceneFieldData{SceneField::Mesh, dataIn, stridedArrayView(dataSlightlyOut).flipped<0>()} + }}; + SceneData{SceneMappingType::UnsignedByte, 6, Containers::Array{24}, { + SceneFieldData{SceneField::Mesh, 6, SceneMappingType::UnsignedByte, 0, 4, SceneFieldType::UnsignedByte, 24, -4} + }}; CORRADE_COMPARE(out.str(), "Trade::SceneData: field data [0xbaddaa:0xbaddb4] of field 0 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: field data [0xdead:0xdeb7] of field 1 are not contained in passed data array [0xbadda9:0xbaddb3]\n" @@ -1917,6 +1938,9 @@ void SceneDataTest::constructFieldDataNotContained() { "Trade::SceneData: field data [0xdead:0xdeb7] of field 1 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: offset-only field data of field 0 span 25 bytes but passed data array has only 24\n" + "Trade::SceneData: offset-only field data of field 0 span 25 bytes but passed data array has only 24\n" + + "Trade::SceneData: field data [0xbaddaa:0xbaddb4] of field 0 are not contained in passed data array [0xbadda9:0xbaddb3]\n" "Trade::SceneData: offset-only field data of field 0 span 25 bytes but passed data array has only 24\n"); }