Critical silent bug: vector push back of an own element moves it and calls a copy constructor on the invalidated reference

[vdemichev]

Below steps to reproduce (MSVC 2022):

#include <iostream>

#include "EASTL/vector.h"

inline void* operator new[](size_t size, const char* pName, int flags, unsigned debugFlags, const char* file, int line) {
    return std::malloc(size);
}
inline void* operator new[](size_t size, size_t alignment, size_t alignmentOffset, const char* pName, int flags, unsigned debugFlags, const char* file, int line) {
    return _aligned_malloc(size, alignment);
}

struct S {
    std::string s;
};

int main() {
    S s = { "Hello World!" };
    eastl::vector<S> v;
    v.push_back(s);
    v.push_back(v.back());

    for (auto& e : v) {
        std::cout << e.s << std::endl;
    }

    return 0;
}

Expected output:

Hello World! Hello World!

Actual output:

Hello World!

What happens: void vector<T, Allocator>::DoInsertValueEnd(Args&&... args) does this:

pointer pNewEnd = eastl::uninitialized_move_ptr_if_noexcept(mpBegin, mpEnd, pNewData);
::new ((void*)pNewEnd) value_type(eastl::forward<Args>(args)...);
pNewEnd++;

If realloc is needed, the vector contents are moved to the newly allocated chunk of memory. This invalidates the previously obtained reference to the object that is to be pushed back. Then a copy constructor is called using this invalid reference. All this happens silently, i.e. leads to difficult to track critical bugs in the program. I checked the commit history, the bug seems to have been there for years.

Btw, void vector<T, Allocator>::DoInsertValue(const_iterator position, Args&&... args) works correctly:

::new((void*)(pNewData + nPosSize)) value_type(eastl::forward<Args>(args)...);                  // Because the old data is potentially being moved rather than copied, we need to move 
pointer pNewEnd = eastl::uninitialized_move_ptr_if_noexcept(mpBegin, destPosition, pNewData);   // the value first, because it might possibly be a reference to the old data being moved.
pNewEnd = eastl::uninitialized_move_ptr_if_noexcept(destPosition, mpEnd, ++pNewEnd);            // Question: with exceptions disabled, do we assume all operations are noexcept and thus there's no need for uninitialized_move_ptr_if_noexcept?

[questor]

I have a fix, but not a fully one, because of that also no pull-request. First a test to verify the behaviour: add this to TestVector:

    // Issue #524 check
    {
        struct S {
            std::string s;
        };
        {
            S s = {"Hello World!"};
            eastl::vector<S> v;
            v.pushBack(s);
            v.pushBack(v.back());

            VERIFY(v[0].s.compare("Hello World!") == 0);
            VERIFY(v[1].s.compare("Hello World!") == 0);

            VERIFY(v.size()==2);
        }
    }

the fix is (approx line 1946 in vector.h):

            //here #524 is fixed!
            ::new((void*)(pNewData+(mpEnd-mpBegin))) value_type(eastl::forward<Args>(args)...);             //first construct the new element in the destination buffer because the source might get moved away by the next line
            pointer pNewEnd = eastl::uninitializedMove_ptr_if_noexcept(mpBegin, mpEnd, pNewData);           //and here copy the rest of the elements into the new buffer
            pNewEnd++;

BUT this will only fix the issue if you have exceptions disabled, when they're enabled it's NOT fixed (and I'm not confident enough to also fix the behaviour here).