ic
/
magnum
mirror of https://github.com/mosra/magnum.git
6
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

Trade: fix SceneData field containment checks for negative strides. 

Why didn't I think of extracting the common code earlier?
pull/549/head
Vladimír Vondruš 4 years ago
parent
f685673be9
commit
68d15bd8d0
2 changed files with 54 additions and 20 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 38
      src/Magnum/Trade/SceneData.cpp
  2. 36
      src/Magnum/Trade/Test/SceneDataTest.cpp

38
src/Magnum/Trade/SceneData.cpp
Unescape View File

@ -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<const char*>(field._mappingData.pointer) + (field._size - 1)*field._mappingStride + mappingTypeSize;
const void* const fieldEnd = static_cast<const char*>(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<const void*>(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast<const void*>(_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<const void*>(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast<const void*>(_data.end()) << Debug::nospace << "]", );
CORRADE_ASSERT(reinterpret_cast<const void*>(mappingBegin) >= _data.begin() && reinterpret_cast<const void*>(mappingEnd) <= _data.end(),
"Trade::SceneData: mapping data [" << Debug::nospace << reinterpret_cast<const void*>(mappingBegin) << Debug::nospace << ":" << Debug::nospace << reinterpret_cast<const void*>(mappingEnd) << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast<const void*>(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast<const void*>(_data.end()) << Debug::nospace << "]", );
CORRADE_ASSERT(reinterpret_cast<const void*>(fieldBegin) >= _data.begin() && reinterpret_cast<const void*>(fieldEnd) <= _data.end(),
"Trade::SceneData: field data [" << Debug::nospace << reinterpret_cast<const void*>(fieldBegin) << Debug::nospace << ":" << Debug::nospace << reinterpret_cast<const void*>(fieldEnd) << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast<const void*>(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast<const void*>(_data.end()) << Debug::nospace << "]", );
}
}
#endif

36
src/Magnum/Trade/Test/SceneDataTest.cpp
Unescape View File

@ -1826,7 +1826,8 @@ void SceneDataTest::constructMappingDataNotContained() {
CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");
#endif
Containers::Array<char> data{reinterpret_cast<char*>(0xbadda9), 10, [](char*, std::size_t){}};
const Containers::Array<char> data{reinterpret_cast<char*>(0xbadda9), 10, [](char*, std::size_t){}};
Containers::Array<char> sameDataButMovable{reinterpret_cast<char*>(0xbadda9), 10, [](char*, std::size_t){}};
Containers::ArrayView<UnsignedShort> dataIn{reinterpret_cast<UnsignedShort*>(0xbadda9), 5};
Containers::ArrayView<UnsignedShort> dataSlightlyOut{reinterpret_cast<UnsignedShort*>(0xbaddaa), 5};
Containers::ArrayView<UnsignedShort> dataOut{reinterpret_cast<UnsignedShort*>(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<char>{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<char>{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<char> data{reinterpret_cast<char*>(0xbadda9), 10, [](char*, std::size_t){}};
const Containers::Array<char> data{reinterpret_cast<char*>(0xbadda9), 10, [](char*, std::size_t){}};
Containers::Array<char> sameDataButMovable{reinterpret_cast<char*>(0xbadda9), 10, [](char*, std::size_t){}};
Containers::ArrayView<UnsignedShort> dataIn{reinterpret_cast<UnsignedShort*>(0xbadda9), 5};
Containers::ArrayView<UnsignedShort> dataSlightlyOut{reinterpret_cast<UnsignedShort*>(0xbaddaa), 5};
Containers::ArrayView<UnsignedShort> dataOut{reinterpret_cast<UnsignedShort*>(0xdead), 5};
@ -1895,13 +1908,13 @@ void SceneDataTest::constructFieldDataNotContained() {
SceneFieldData{sceneFieldCustom(37), dataIn.prefix(2), Containers::StridedArrayView2D<UnsignedByte>{Containers::ArrayView<UnsignedByte>{reinterpret_cast<UnsignedByte*>(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<char>{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<char>{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<char>{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");
}

Write Preview
Loading…
Cancel
Save