From 6465fa14c9762a79a7f1eb085c574827be201dae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 21 Jun 2018 14:20:03 +0200 Subject: [PATCH] GL: fix undefined behavior and potential leaks in Mesh. The change done in 680144f1c511997ee0530ea88bbb95aa20e058c8 was not properly handling these cases: * Mesh(NoCreateT) and wrap() were not constructing the internal vector, which blew up when move-assigning another instance. * ~Mesh() was not destructing the internal vector if the VAO ID was zero or non-owning wrap() was used. Strangely enough none of these were causing *any* problems for me on Linux (even ASan was totally quiet and due to the unfortunate combination of bugs even when I assigned totally random data to the storage vector). This however blew up on MSVC, assuming there the implementation is more checked. Because it's possible to construct Mesh with no GL context available, the move construction and destruction needs to avoid accessing Context unless really necessary (it would be also unclear which type of vector should be constructed if we have no context). Extended the tests to handle hopefully all the cases. --- src/Magnum/GL/Implementation/MeshState.h | 4 +- src/Magnum/GL/Mesh.cpp | 104 ++++++++++++++++------- src/Magnum/GL/Mesh.h | 17 ++-- src/Magnum/GL/Test/MeshGLTest.cpp | 14 ++- src/Magnum/GL/Test/MeshTest.cpp | 19 +++++ 5 files changed, 116 insertions(+), 42 deletions(-) diff --git a/src/Magnum/GL/Implementation/MeshState.h b/src/Magnum/GL/Implementation/MeshState.h index e5f778e70..9868483c6 100644 --- a/src/Magnum/GL/Implementation/MeshState.h +++ b/src/Magnum/GL/Implementation/MeshState.h @@ -40,10 +40,10 @@ struct MeshState { void reset(); - void(Mesh::*createImplementation)(); + void(Mesh::*createImplementation)(bool); void(Mesh::*moveConstructImplementation)(Mesh&&); void(Mesh::*moveAssignImplementation)(Mesh&&); - void(Mesh::*destroyImplementation)(); + void(Mesh::*destroyImplementation)(bool); void(Mesh::*attributePointerImplementation)(Mesh::AttributeLayout&&); #if !defined(MAGNUM_TARGET_GLES) || defined(MAGNUM_TARGET_GLES2) void(Mesh::*vertexAttribDivisorImplementation)(GLuint, GLuint); diff --git a/src/Magnum/GL/Mesh.cpp b/src/Magnum/GL/Mesh.cpp index 60e8680eb..a09e85ac8 100644 --- a/src/Magnum/GL/Mesh.cpp +++ b/src/Magnum/GL/Mesh.cpp @@ -208,20 +208,22 @@ std::size_t Mesh::indexSize(Magnum::MeshIndexType type) { #endif Mesh::Mesh(const MeshPrimitive primitive): _primitive{primitive}, _flags{ObjectFlag::DeleteOnDestruction} { - (this->*Context::current().state().mesh->createImplementation)(); + (this->*Context::current().state().mesh->createImplementation)(true); } Mesh::Mesh(NoCreateT) noexcept: _id{0}, _primitive{MeshPrimitive::Triangles}, _flags{ObjectFlag::DeleteOnDestruction} {} Mesh::~Mesh() { - /* Moved out or not deleting on destruction, nothing to do */ - if(!_id || !(_flags & ObjectFlag::DeleteOnDestruction)) return; + const bool deleteObject = _id && (_flags & ObjectFlag::DeleteOnDestruction); - /* Remove current vao from the state */ - GLuint& current = Context::current().state().mesh->currentVAO; - if(current == _id) current = 0; + /* Moved out or not deleting on destruction, nothing to do */ + if(deleteObject) { + /* Remove current vao from the state */ + GLuint& current = Context::current().state().mesh->currentVAO; + if(current == _id) current = 0; + } - (this->*Context::current().state().mesh->destroyImplementation)(); + if(_constructed) (this->*Context::current().state().mesh->destroyImplementation)(deleteObject); } Mesh::Mesh(Mesh&& other) noexcept: _id(other._id), _primitive(other._primitive), _flags{other._flags}, _countSet{other._countSet}, _count(other._count), _baseVertex{other._baseVertex}, _instanceCount{other._instanceCount}, @@ -233,7 +235,8 @@ Mesh::Mesh(Mesh&& other) noexcept: _id(other._id), _primitive(other._primitive), #endif _indexOffset(other._indexOffset), _indexType(other._indexType), _indexBuffer{std::move(other._indexBuffer)} { - (this->*Context::current().state().mesh->moveConstructImplementation)(std::move(other)); + if(_constructed || other._constructed) + (this->*Context::current().state().mesh->moveConstructImplementation)(std::move(other)); other._id = 0; } @@ -256,12 +259,16 @@ Mesh& Mesh::operator=(Mesh&& other) noexcept { swap(_indexOffset, other._indexOffset); swap(_indexType, other._indexType); swap(_indexBuffer, other._indexBuffer); - (this->*Context::current().state().mesh->moveAssignImplementation)(std::move(other)); + + if(_constructed || other._constructed) + (this->*Context::current().state().mesh->moveAssignImplementation)(std::move(other)); return *this; } -Mesh::Mesh(const GLuint id, const MeshPrimitive primitive, const ObjectFlags flags): _id{id}, _primitive{primitive}, _flags{flags} {} +Mesh::Mesh(const GLuint id, const MeshPrimitive primitive, const ObjectFlags flags): _id{id}, _primitive{primitive}, _flags{flags} { + (this->*Context::current().state().mesh->createImplementation)(false); +} inline void Mesh::createIfNotAlready() { /* If VAO extension is not available, the following is always true */ @@ -538,67 +545,100 @@ void Mesh::bindVAO() { } } -void Mesh::createImplementationDefault() { +void Mesh::createImplementationDefault(bool) { _id = 0; _flags |= ObjectFlag::Created; static_assert(sizeof(_attributes) >= sizeof(std::vector), "attribute storage buffer size too small"); new(&_attributes) std::vector; + _constructed = true; } -void Mesh::createImplementationVAO() { - #ifndef MAGNUM_TARGET_GLES2 - glGenVertexArrays(1, &_id); - #else - glGenVertexArraysOES(1, &_id); - #endif - CORRADE_INTERNAL_ASSERT(_id != Implementation::State::DisengagedBinding); +void Mesh::createImplementationVAO(const bool createObject) { + if(createObject) { + #ifndef MAGNUM_TARGET_GLES2 + glGenVertexArrays(1, &_id); + #else + glGenVertexArraysOES(1, &_id); + #endif + CORRADE_INTERNAL_ASSERT(_id != Implementation::State::DisengagedBinding); + } static_assert(sizeof(_attributes) >= sizeof(std::vector), "attribute storage buffer size too small"); new(&_attributes) std::vector; + _constructed = true; } #ifndef MAGNUM_TARGET_GLES -void Mesh::createImplementationVAODSA() { - glCreateVertexArrays(1, &_id); - _flags |= ObjectFlag::Created; +void Mesh::createImplementationVAODSA(const bool createObject) { + if(createObject) { + glCreateVertexArrays(1, &_id); + _flags |= ObjectFlag::Created; + } static_assert(sizeof(_attributes) >= sizeof(std::vector), "attribute storage buffer size too small"); new(&_attributes) std::vector; + _constructed = true; } #endif void Mesh::moveConstructImplementationDefault(Mesh&& other) { new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + _constructed = true; } void Mesh::moveConstructImplementationVAO(Mesh&& other) { new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + _constructed = true; } +/* In the move construction / assignment we need to stay noexcept, which means + it's only possible to swap or move-construct the vector, can't delete or + allocate. So, in the particular case where `this` is constructed and `other` + is not, we do a "partial swap" -- `other` gets our data and our data gets + emptied. */ + void Mesh::moveAssignImplementationDefault(Mesh&& other) { - std::swap(*reinterpret_cast*>(&_attributes), - *reinterpret_cast*>(&other._attributes)); + if(_constructed && other._constructed) + std::swap(*reinterpret_cast*>(&_attributes), + *reinterpret_cast*>(&other._attributes)); + else if(_constructed && !other._constructed) { + other._constructed = true; + new(&other._attributes) std::vector{std::move(*reinterpret_cast*>(&_attributes))}; + } else if(!_constructed && other._constructed) { + _constructed = true; + new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + } } void Mesh::moveAssignImplementationVAO(Mesh&& other) { - std::swap(*reinterpret_cast*>(&_attributes), - *reinterpret_cast*>(&other._attributes)); + if(_constructed && other._constructed) + std::swap(*reinterpret_cast*>(&_attributes), + *reinterpret_cast*>(&other._attributes)); + else if(_constructed && !other._constructed) { + other._constructed = true; + new(&other._attributes) std::vector{std::move(*reinterpret_cast*>(&_attributes))}; + } else if(!_constructed && other._constructed) { + _constructed = true; + new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + } } -void Mesh::destroyImplementationDefault() { +void Mesh::destroyImplementationDefault(bool) { reinterpret_cast*>(&_attributes)->~vector(); } -void Mesh::destroyImplementationVAO() { - #ifndef MAGNUM_TARGET_GLES2 - glDeleteVertexArrays(1, &_id); - #else - glDeleteVertexArraysOES(1, &_id); - #endif +void Mesh::destroyImplementationVAO(const bool deleteObject) { + if(deleteObject) { + #ifndef MAGNUM_TARGET_GLES2 + glDeleteVertexArrays(1, &_id); + #else + glDeleteVertexArraysOES(1, &_id); + #endif + } reinterpret_cast*>(&_attributes)->~vector(); } diff --git a/src/Magnum/GL/Mesh.h b/src/Magnum/GL/Mesh.h index 0508f7880..56cce077b 100644 --- a/src/Magnum/GL/Mesh.h +++ b/src/Magnum/GL/Mesh.h @@ -1084,10 +1084,10 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { void drawInternal(TransformFeedback& xfb, UnsignedInt stream, Int instanceCount); #endif - void MAGNUM_GL_LOCAL createImplementationDefault(); - void MAGNUM_GL_LOCAL createImplementationVAO(); + void MAGNUM_GL_LOCAL createImplementationDefault(bool); + void MAGNUM_GL_LOCAL createImplementationVAO(bool createObject); #ifndef MAGNUM_TARGET_GLES - void MAGNUM_GL_LOCAL createImplementationVAODSA(); + void MAGNUM_GL_LOCAL createImplementationVAODSA(bool createObject); #endif void MAGNUM_GL_LOCAL moveConstructImplementationDefault(Mesh&& other); @@ -1095,8 +1095,8 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { void MAGNUM_GL_LOCAL moveAssignImplementationDefault(Mesh&& other); void MAGNUM_GL_LOCAL moveAssignImplementationVAO(Mesh&& other); - void MAGNUM_GL_LOCAL destroyImplementationDefault(); - void MAGNUM_GL_LOCAL destroyImplementationVAO(); + void MAGNUM_GL_LOCAL destroyImplementationDefault(bool); + void MAGNUM_GL_LOCAL destroyImplementationVAO(bool deleteObject); void attributePointerInternal(const Buffer& buffer, GLuint location, GLint size, GLenum type, DynamicAttribute::Kind kind, GLintptr offset, GLsizei stride, GLuint divisor); void MAGNUM_GL_LOCAL attributePointerInternal(AttributeLayout&& attribute); @@ -1149,9 +1149,12 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { GLuint _id; MeshPrimitive _primitive; ObjectFlags _flags; - /* using a separate bool instead of Optional to make use of the 3-byte - gap after _flags */ + /* using a separate bool for _count instead of Optional to make use of + the 3-byte gap after _flags */ bool _countSet{}; + /* Whether the _attributes storage was constructed (it's not when the + object is constructed using NoCreate). Also fits in the gap. */ + bool _constructed{}; Int _count{}, _baseVertex{}, _instanceCount{1}; #ifndef MAGNUM_TARGET_GLES UnsignedInt _baseInstance{}; diff --git a/src/Magnum/GL/Test/MeshGLTest.cpp b/src/Magnum/GL/Test/MeshGLTest.cpp index d09ebb229..db1de0fb2 100644 --- a/src/Magnum/GL/Test/MeshGLTest.cpp +++ b/src/Magnum/GL/Test/MeshGLTest.cpp @@ -328,11 +328,13 @@ void MeshGLTest::constructMove() { CORRADE_VERIFY(id > 0); } + /* Move construct */ Mesh b(std::move(a)); CORRADE_COMPARE(a.id(), 0); CORRADE_COMPARE(b.id(), id); + /* Move assign */ Mesh c; c.addVertexBuffer(buffer2, 1, Attribute<1, Float>{}); const Int cId = c.id(); @@ -352,6 +354,16 @@ void MeshGLTest::constructMove() { CORRADE_COMPARE(b.id(), cId); CORRADE_COMPARE(c.id(), id); + /* Move assign to a NoCreate instance */ + Mesh d{NoCreate}; + d = std::move(c); + + CORRADE_COMPARE(c.id(), 0); + CORRADE_COMPARE(d.id(), id); + + /* Destroy */ + b = Mesh{NoCreate}; + /* Test that drawing still works properly */ { MAGNUM_VERIFY_NO_GL_ERROR(); @@ -369,7 +381,7 @@ void MeshGLTest::constructMove() { .bind(); FloatShader shader{"float", "vec4(valueInterpolated, 0.0, 0.0, 0.0)"}; - c.setPrimitive(MeshPrimitive::Points) + d.setPrimitive(MeshPrimitive::Points) .setCount(1) .draw(shader); diff --git a/src/Magnum/GL/Test/MeshTest.cpp b/src/Magnum/GL/Test/MeshTest.cpp index 3d189f91c..79bb0ef13 100644 --- a/src/Magnum/GL/Test/MeshTest.cpp +++ b/src/Magnum/GL/Test/MeshTest.cpp @@ -43,6 +43,7 @@ struct MeshTest: TestSuite::Tester { void constructViewNoCreate(); void constructCopy(); + void constructMoveNoCreate(); /* View *is* copyable */ void drawCountNotSet(); @@ -69,6 +70,7 @@ MeshTest::MeshTest() { &MeshTest::constructViewNoCreate, &MeshTest::constructCopy, + &MeshTest::constructMoveNoCreate, &MeshTest::drawCountNotSet, &MeshTest::drawViewCountNotSet, @@ -113,6 +115,23 @@ void MeshTest::constructCopy() { CORRADE_VERIFY(!(std::is_assignable{})); } +void MeshTest::constructMoveNoCreate() { + /* Neither of these should be accessing the GL context */ + { + Mesh a{NoCreate}; + CORRADE_COMPARE(a.id(), 0); + + Mesh b{std::move(a)}; + CORRADE_COMPARE(b.id(), 0); + + Mesh c{NoCreate}; + c = std::move(b); + CORRADE_COMPARE(c.id(), 0); + } + + CORRADE_VERIFY(true); +} + namespace { struct Shader: AbstractShaderProgram { explicit Shader(NoCreateT): AbstractShaderProgram{NoCreate} {}