free(): invalid pointer when implementing erase() of vector

Question

I'm trying to implement Vector class. For a starter my Vector will not yet support generics, only my Thing class.

The Vector class should support: empty(), push_back(), erase() and subscript operator. It should also resize accordingly if needed.

My Vector implementation:

class Vect {
public:
    Vect() {
        length = 0;
        capacity = 3;
        charCnt = 0;
        data = new Thing[capacity];
    }
    ~Vect() {
        delete[] data;
    }

    bool empty() const {
        return length == 0;
    }

    void push_back(const char *str) {
        if(length + 1 == capacity)
            doubleSize();
        data[length++] = Thing(str);
        charCnt += strlen(str);
    }

    bool erase(size_t at) {
        if(at >= length)
            return false;

        auto newData = new Thing[capacity];
        size_t newIndex = 0;

        for(size_t i = 0; i < at; i++)
            newData[newIndex++] = data[i];

        for(size_t i = at + 1; i < length; i++)
            newData[newIndex++] = data[i];

        //free(): invalid pointer
        delete[] data;
        data = newData;

        return true;
    }

    const char* operator[](unsigned int index) {
        return data[index].getStr();
    }

    char* toString() {
        auto result = make_shared<char *>(new char[charCnt + 1]);
        size_t resultIndex = 0;

        for(size_t dataIndex = 0; dataIndex < length; dataIndex++) {
            auto patchOffset = data[dataIndex].getO();
            auto patchLength = data[dataIndex].getL();
            for(size_t patchIndex = patchOffset; patchIndex < patchLength; patchIndex++)
                (*result.get())[resultIndex++] = data[dataIndex].getStr()[patchIndex];
        }

        (*result.get())[resultIndex] = '\0';
        return *result.get();
    }

private:
    size_t length, capacity, charCnt;
    Thing *data;

    void doubleSize() {
        size_t newCapacity = capacity*2;
        auto newData = new Thing[newCapacity];

        for(size_t i = 0; i < length; i++) {
            newData[i] = data[i];
        }

        //this works
        delete[] data;
        data = newData;
    }
};

I've ran into problems when I tried implementing erase(). My implementation is straight forward: erase() takes one argument, the index at which the element should be removed. So I create a new array, copy all the contents up to the erasion-index, skip the index, and copy the rest. Then I delete the old array and assign new array to the variable.

I did something very similar in doubleSize() method, which seems to be working fine (checked valgrind).

The problem I've ran into is that delete[] doesn't work on data.

The test environment I used:

class Thing {
public:
    Thing() {
        o = 0;
        l = 0;
        ptr = nullptr;
    }

    explicit Thing(const char *str) {
        ptr = str;
        o = 0;
        l = strlen(str);
    }

    Thing(const Thing &other) {
        this->o = other.o;
        this->l = other.l;
        this->ptr = other.ptr;
    }
    friend void swap(Thing &first, Thing &other) {
        using std::swap;
        swap(first.o, other.o);
        swap(first.l, other.l);
        swap(first.ptr, other.ptr);
    }
    Thing& operator=(Thing other) {
        swap(*this, other);
        return *this;
    }

    Thing(Thing &&other) noexcept: Thing() {
        swap(*this, other);
    }

    size_t getO() const {
        return o;
    }
    size_t getL() const {
        return l;
    }

    const char* getStr() const {
        return ptr;
    }
private:
    size_t o, l;
    const char *ptr;
};

//class Vect...

int main() {
    Vect s; char tmpStr[100];

    assert(s.empty());

    s.push_back("hello ");
    s.push_back("world");
    s.push_back("!");
    s.push_back(" this ");
    s.push_back("is ");
    s.push_back("me!");

    strncpy(tmpStr, "hello world! this is me!", sizeof(tmpStr));
    assert(stringMatch(s.toString(), tmpStr));

    s.erase(2);
    strncpy(tmpStr, "hello world this is me!", sizeof(tmpStr));
    assert(stringMatch(s.toString(), tmpStr));
}

Upon consulting the debugger I've found the following:

1. first for loop works fine as content is concerned.

2. second loop: after second iteration, after the assignment - data[0] gets corrupted - o and l variables get random value while ptr still points to the correct string.

3. after finishing the last iteration of second loop, ptr now becomes NULL and data[1] now has random o and l values.

Afterwards delete [] gets called which will unsurprisingly trigger an error because I did some wild rodeo with the allocated memory.

Where did I mismanage my memory?

Answer 1

doubleSize allocates new buffer, but doesn't update capacity member. Oncelength is past capacity, you don't even detect overflow anymore. Eventually, you'll overflow for real.

---

Although you are erasing using erase, this reduces the size of the vector, but it doesn't decrement your length variable.