GL: fix undefined behavior and potential leaks in Mesh.

The change done in 680144f1c5 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

@@ -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);

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<AttributeLayout>),
         "attribute storage buffer size too small");
     new(&_attributes) std::vector<AttributeLayout>;
+    _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<Buffer>),
         "attribute storage buffer size too small");
     new(&_attributes) std::vector<Buffer>;
+    _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<Buffer>),
         "attribute storage buffer size too small");
     new(&_attributes) std::vector<Buffer>;
+    _constructed = true;
 }
 #endif
 
 void Mesh::moveConstructImplementationDefault(Mesh&& other) {
     new(&_attributes) std::vector<AttributeLayout>{std::move(*reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes))};
+    _constructed = true;
 }
 
 void Mesh::moveConstructImplementationVAO(Mesh&& other) {
     new(&_attributes) std::vector<Buffer>{std::move(*reinterpret_cast<std::vector<Buffer>*>(&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<std::vector<AttributeLayout>*>(&_attributes),
-        *reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes));
+    if(_constructed && other._constructed)
+        std::swap(*reinterpret_cast<std::vector<AttributeLayout>*>(&_attributes),
+            *reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes));
+    else if(_constructed && !other._constructed) {
+        other._constructed = true;
+        new(&other._attributes) std::vector<AttributeLayout>{std::move(*reinterpret_cast<std::vector<AttributeLayout>*>(&_attributes))};
+    } else if(!_constructed && other._constructed) {
+        _constructed = true;
+        new(&_attributes) std::vector<AttributeLayout>{std::move(*reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes))};
+    }
 }
 
 void Mesh::moveAssignImplementationVAO(Mesh&& other) {
-    std::swap(*reinterpret_cast<std::vector<Buffer>*>(&_attributes),
-        *reinterpret_cast<std::vector<Buffer>*>(&other._attributes));
+    if(_constructed && other._constructed)
+        std::swap(*reinterpret_cast<std::vector<Buffer>*>(&_attributes),
+            *reinterpret_cast<std::vector<Buffer>*>(&other._attributes));
+    else if(_constructed && !other._constructed) {
+        other._constructed = true;
+        new(&other._attributes) std::vector<Buffer>{std::move(*reinterpret_cast<std::vector<Buffer>*>(&_attributes))};
+    } else if(!_constructed && other._constructed) {
+        _constructed = true;
+        new(&_attributes) std::vector<Buffer>{std::move(*reinterpret_cast<std::vector<Buffer>*>(&other._attributes))};
+    }
 }
 
-void Mesh::destroyImplementationDefault() {
+void Mesh::destroyImplementationDefault(bool) {
     reinterpret_cast<std::vector<AttributeLayout>*>(&_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<std::vector<Buffer>*>(&_attributes)->~vector();
 }

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{};

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);
 

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<Mesh, const Mesh&>{}));
 }
 
+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} {}