Memory overwrite in my own Vector class

Question

For my Algorithm course project we can't use STL stuff like std::vector and so I'm trying to implement my own version of it (with templates).

It seems it works but when I declare a Vector< Vector< int > > the .push() method starts to overwrite memory.

More specifically, with this code:

Vector<Vector<int>> v(3);

cout << v[0].push(0) << "\n";
cout << v[0].push(55) << "\n";
cout << v[0].push(4) << "\n";
cout << v[1].push(12) << "\n";
cout << v[1].push(3) << "\n";
cout << v[2].push(1) << "\n";

The output is this (.push() returns the address of where the element is inserted):

0x561328b0bc20
0x561328b0bc24
0x561328b0bc28
0x561328b0bc20
0x561328b0bc24
0x561328b0bc20

Any suggestion of why this happens?

Here is the code for my Vector class:

#include <iostream>
#include <bits/stdc++.h>
using namespace std;

template<class T>
class Vector {
private:
    size_t  _size;
    size_t  _capacity;
    char*   _buffer; //char* for performance

    void _realloc(size_t);

public:
    Vector(size_t s=0, T def=T());

    T* push(T);
    T& operator[](int);
    size_t size() { return _size; }

};

template<class T>
void Vector<T>:: _realloc(size_t ncap)
{
    _capacity = ncap;
    char* nbuf = _buffer;
    _buffer = new char[_capacity];
    for(size_t i=0; i<_size * sizeof(T); ++i)
        _buffer[i] = nbuf[i];
    delete[] nbuf;
}

/*
 * s -> size
 * def -> default value
 */
template<class T>
Vector<T>:: Vector(size_t s, T def) : _size(s)
{
    _capacity = 32;
    while(_capacity < _size)
        _capacity *= 2;

    _buffer = new char[_capacity * sizeof(T)];
    for(size_t i=0; i<_size; ++i)
        ((T*)_buffer)[i] = def;
}

/*
 * check capacity, reallocs if necessary and push the element
 * then return the addres (used only for debug)
 */
template<class T>
T* Vector<T>:: push(T ele)
{
    if(_capacity == _size)
        _realloc(2 * _capacity);
    ((T*)_buffer)[_size++] = ele;

    return &((T*)_buffer)[_size-1];
}

template<class T>
T& Vector<T>:: operator[](int i)
{
    if(i<0 or i>=(int)_size) {
        cerr << "Out of bounds!\n";
        abort();
    }else
        return ((T*)_buffer)[i];
}

template<class T>
ostream& operator<<(ostream& out, Vector<T>& v)
{
    out << "{";
    if(v.size() > 0) {
        out << v[0];
        for(size_t i=1; i<v.size(); ++i)
            out << ", " << v[i];
    }
    out << "}";
    return out;
}

Thanks!

PS: I know it's not a good use of C++ :P

[The Rule of Three/Five/Zero](http://en.cppreference.com/w/cpp/language/rule_of_three). — WhozCraig, May 11 '17 at 19:27
Please, make a [mcve]. If the problem does not solve itself in the process, we will be able to help you — Amadeus, May 11 '17 at 19:28
In case you're wondering, yes, I accidently skipped over the link when reading your question. Sorry for that. Please do take care to put all relevant information inside your questions, though. — Quentin, May 11 '17 at 19:35
Well, @Quentin fixed my problem with the question about 5 seconds after I voted it closed. Voting to reopen. Off topic: `#include ` combined with `using namespace std;` is a sneaky bug factory. Do not use the `#include` ([many reasons](http://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h)) and give [Why is “using namespace std” considered bad practice?](http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) a read. — user4581301, May 11 '17 at 19:41
Also be careful with where and how you use underscore prefixes. [They often have special meaning in C++](http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier) — user4581301, May 11 '17 at 19:43
@OP -- There are a lot of issues with your code. 1) No user-defined copy constructor, assignment operator, or destructor. 2) Resizing the vector mutates its members *before* the new memory has been safely allocated. If `new` throws an exception, your object has been corrupted. 3) Don't start your variable names with underscores. 4) Calling `abort` just because an index is out of bounds. Throw an exception, don't shut the program down. I could list more, but instead of that [see this implementation](http://coliru.stacked-crooked.com/a/0b1eaa1bfd7c6c52) and compare what you did with this. — PaulMcKenzie, May 11 '17 at 19:43
@PaulMcKenzie underscore-prefixed names without a capital are fine if they're not in the global namespace. In fact, it's a common marker for member variables. — Quentin, May 11 '17 at 19:45
@Quentin Can't remember all of the rules, so the easiest thing for me is to not use them. — PaulMcKenzie, May 11 '17 at 19:51
*It seems it works* -- This does not work: `{Vector v1(10); Vector v2 = v1;}` — PaulMcKenzie, May 11 '17 at 20:03
I'm sorry for my bad habits in coding, my knowledge of C++ is limited to coding contests like Hackerrank. Thanks to all for the patience, I learned a lot even though are basic things! :) — VashTheStampede, May 11 '17 at 21:26

score 2 · Accepted Answer · answered May 11 '17 at 19:26

Your operator= implicitly defined does the wrong thing. You use it in your constructor.

So, follow the rule of 0/3/5: Implement copy/move construtors/assignment and destructors, as this is an owning memory-management type. (Non-resource management types should follow the rule of 0; copyable resource management types the rule of 5.)

See rule of three. Implement all of destructor/copy constructor/move constructor/copy assign/move assign, or none of them.

Don't copy the data byte-wise when you realloc. std::move the Ts from the source to the dest.

In the copy construct/assign you'll want to copy the source Ts, not the underlying bytes.

Thank you for your patience, it looks like I lack the basics. With both of you and the others I learned something new! — VashTheStampede, May 11 '17 at 21:31

Memory overwrite in my own Vector class

1 Answers1