Trouble with destructor using

Question

I started learning c ++ and I ran into this problem. If I do not use the destructor, then everything works fine, but when I add it, the following error occurs:

*** Error in `./arraylist.o': double free or corruption (fasttop): 0xb71ab360 ***
./test_arraylist.sh: line 2:   513 Aborted                 ./arraylist.o

My source code:

#include <iostream>
#include "jppsys.h"

namespace jpparl {
    template <class T> class ArrayList {
        private:
            T *array;
            int step;
            long totalSize;
            long usedSize = 0;
        public:
            ArrayList(long step) {
                this->step = step;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList() {
                this->step = 8;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList(const ArrayList &arraylist) {
                step = arraylist.step;
                totalSize = arraylist.totalSize;
                usedSize = arraylist.usedSize;
                array = new T[totalSize];
                std::copy(arraylist.array, arraylist.array + totalSize, array);
                std::cout << "\nCopy " << array << "\n";
            }

            ~ArrayList() {
                std::cout << "\nDelete " << array << "\n";
                // delete[] array;
                // ^^^^^^^^^^^^^^^ error here
            }

            void add(T elem, long index) {
                if (usedSize == totalSize) totalSize += step;

                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index, array + index + usedSize, tmp + index + 1);
                delete[] array;
                tmp[index] = elem;
                array = tmp;

                usedSize++;
            }

            void remove(long index) {
                if (usedSize == totalSize - step) totalSize -= step;
                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index + 1, array + usedSize, tmp + index);
                delete[] array;
                array = tmp;

                usedSize--;
            }

            T get(long index) {
                return array[index];
            }

            long size() {
                return usedSize;
            }

            long getTotalSize() {
                return totalSize;
            }
    };

}

using namespace jpparl;
using namespace std;
int main() {
    ArrayList<ArrayList<int>> al(1);
    ArrayList<int> al2(1);
    cout << "\nAdding 256\n";
    al2.add(256,0); // al2.get(0) - 256
    cout << "\nAdding al2\n";
    al.add(al2, 0); // adds copy of a2, al.get(0) - copy of.a2
    cout << "\nRemoving 256\n";
    al2.remove(0); // removes 256 from al2, not from a1
    cout << al2.get(0) << " - must be 0      " << al.get(0).get(0) << "     - must be 256\n";
    cout << al2.size() << " - must be 0     " << al.get(0).size() << "     - must be 1\n";
}

And program outputs:

Without destructor:

Create 0xb86d4f30

Create 0xb86d4f18

Create 0xb86d5360

Adding 256

Adding al2

Copy 0xb86d5360

Create 0xb86d53a0

Delete 0xb86d4f30

Delete 0xb86d5360

Removing 256
0 - must be 0
Copy 0xb86d5370
256     - must be 256

Delete 0xb86d5370
0 - must be 0
Copy 0xb86d53d8
1      - must be 1

Delete 0xb86d53d8

Delete 0xb86d53c8

Delete 0xb86d5388

With destructor:

Create 0xb71aaf30

Create 0xb71aaf18

Create 0xb71ab360

Adding 256

Adding al2

Copy 0xb71ab360

Create 0xb71ab3a0

Delete 0xb71aaf30

Delete 0xb71ab360

Removing 256
0 - must be 0
Copy 0xb71ab370
0     - must be 256

Delete 0xb71ab370
0 - must be 0
Copy 0xb71ab370
1     - must be 1

Delete 0xb71ab370

Delete 0xb71ab360

Delete 0xb71ab388

Delete 0xb71ab360
*** Error in `./arraylist.o': double free or corruption (fasttop): 0xb71ab360 ***
./test_arraylist.sh: line 2:   513 Aborted                 ./arraylist.o

I will be grateful for help

Apart from anything else, if you have a destructor and a copy constructor, you almost certainly need an assignment operator too. — , Aug 11 '18 at 19:03
Do the crashes go away if you temporarily comment-out the two calls to `jppsys::JPPSystem::arraycopy()`? If so, that might be a clue that that function is buggy, or is being called with incorrect arguments. — Jeremy Friesner, Aug 11 '18 at 19:06
Voted to close as lacking a **reproducable example**. You can make the example reproducable by replacing `jppsys::JPPSystem::arraycopy` with `std::copy`. — Cheers and hth. - Alf, Aug 11 '18 at 19:10
Uhm, in the revised code, the second argument in `std::copy(array + index, array + index + usedSize, tmp + index + 1)` will generally produce a pointer far beyond the array. However, it looks like that function is only called with index 0. Did you check that this reproduces the earlier behavior? — Cheers and hth. - Alf, Aug 11 '18 at 19:19
@congard *If I do not use the destructor, then everything works fine, but when I add it, the following error occurs:* -- I see this posted many times by new C++ programmers, that if the destructor is removed, everything "works fine". Wrong -- all the call to the destructor did was expose the memory corruption that was going on in your program way before the destructor was invoked. — PaulMcKenzie, Aug 11 '18 at 20:20

Remy Lebeau · Accepted Answer · 2018-08-12T17:02:46.127

Your class does not follow the Rule of 3, which basically states that if a class needs to implement a copy constructor, a copy assignment operator, or a destructor, it likely needs to implement all three (C++11 introduces move semantics by adding a move constructor and move assignment operator as well, thus making the Rule of 5).

Your class has a copy constructor and a destructor, but it is missing a copy assignment operator, which gets called by std::copy() when copying elements that are themselves instances of your class. Your class is also missing a move constructor and move assignment operator, as you are using C++11 (by virtual of how you are initializing your usedSize member).

Also, your add() and remove() methods are not managing the array correctly, either. There is no need to reallocate the array on every add/remove operation. That defeats the whole purpose of your step member. Reallocate only when actually needed (growing/shrinking beyond the current step).

The best option in this situation is to simply use std::vector instead of a manual array, eg:

#include <vector>
#include <utility>
#include "jppsys.h"

namespace jpparl {
    template <class T>
    class ArrayList {
    private:
        std::vector<T> array;

    public:
        void add(const T &elem) {
            array.push_back(elem);
        }

        void add(const T &elem, long index) {
            array.insert(array.begin()+index, std::move(elem));
        }

        void remove(long index) {
            array.erase(array.begin()+index);
        }

        T get(long index) {
            return array[index];
        }

        long size() {
            return array.size();
        }

        long getTotalSize() {
            return array.capacity();
        }
    };
}

If that is not an option for you, if you must use a manual array, then you should utilize the copy-swap idiom when making copies of the array, eg:

#include <algorithm>
#include <utility>
#include "jppsys.h"

namespace jpparl {
    template <class T>
    class ArrayList {
    private:
        T *array;
        int step;
        long totalSize;
        long usedSize = 0;

    public:
        ArrayList(long step = 8) {
            this->step = step;
            totalSize = step;
            array = new T[totalSize];
        }

        ArrayList(const ArrayList &src) {
            step = src.step;
            totalSize = src.totalSize;
            usedSize = src.usedSize;
            array = new T[totalSize];
            std::copy(src.array, src.array + usedSize, array);
        }

        ArrayList(ArrayList &&src) {
            step = src.step;
            totalSize = 0;
            usedSize = 0;
            array = nullptr;
            src.swap(*this);
        }

        ~ArrayList() {
            delete[] array;
        }

        ArrayList& operator=(const ArrayList &rhs) {
            if (&rhs != this)
                ArrayList(rhs).swap(*this);
            return *this;
        }

        ArrayList& operator=(ArrayList &&rhs) {
            rhs.swap(*this);
            return *this;
        }

        void swap(ArrayList &other) {
            std::swap(step, other.step);
            std::swap(totalSize, other.totalSize);
            std::swap(usedSize, other.usedSize);
            std::swap(array, other.array);
        }

        void add(const T &elem, long index = -1) {
            if (index == -1) index = usedSize;
            if (usedSize == totalSize) {
                ArrayList tmp(totalSize + step);
                std::copy(array, array + usedSize, tmp.array);
                std::swap(array, tmp.array);
                totalSize = tmp.totalSize;
            }
            std::copy_backward(array + index, array + index + (usedSize - index), array + usedSize + 1);
            array[index] = elem;
            ++usedSize;
        }

        void remove(long index) {
            std::copy(array + index + 1, array + usedSize, array + index);
            --usedSize;
            if ((usedSize == (totalSize - step)) && (totalSize > step)) {
                ArrayList tmp(totalSize - step);
                std::copy(array, array + usedSize, tmp.array);
                std::swap(array, tmp.array);
                totalSize = tmp.totalSize;
            }
        }

        T get(long index) {
            return array[index];
        }

        long size() {
            return usedSize;
        }

        long getTotalSize() {
            return totalSize;
        }
    };
}

Can you explain please, why I need use `std::swap` in functions `add` and `remove` instead `std::copy` and `delete[]`? — congard, Aug 12 '18 at 12:23
@congard did you read up about the copy-swap idiom? A temp `ArrayList` is created to allocate a new array, the existing elements are copied to the new array, and then the old and new array pointers are swapped. The temp object takes ownership of the old array to free it when going out of scope. The main object takes ownership of the new array. This approach ensures that both pointers are owned at all times, and freed properly, especially if any exceptions occur. — Remy Lebeau, Aug 12 '18 at 17:11

score 1 · Answer 2 · answered Aug 11 '18 at 19:33

The asignment operator is missing. In the line

al.add(al2, 0);

al2 Is shallow copied, meaning that the two objects share the same buffer. It then gets deleted twice. Once after the temporary that was passed to the add method is destroyed and once at the end of the main function.

score 0 · Answer 3 · answered Aug 11 '18 at 19:32

As Neil Butterworth said, I needed to add an assignment operator.

#include <iostream>

namespace jpparl {
    template <class T> class ArrayList {
        private:
            T *array;
            int step;
            long totalSize;
            long usedSize = 0;

            void copy(const ArrayList &arraylist) {
                step = arraylist.step;
                totalSize = arraylist.totalSize;
                usedSize = arraylist.usedSize;
                array = new T[totalSize];
                std::copy(arraylist.array, arraylist.array + totalSize, array);
                std::cout << "\nCopy " << array << "\n";
            }

        public:
            ArrayList(long step) {
                this->step = step;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList() {
                this->step = 8;
                totalSize = step;
                array = new T[step];
                std::cout << "\nCreate " << array << "\n";
            }

            ArrayList(const ArrayList &arraylist) {
                copy(arraylist);
            }

            ~ArrayList() {
                std::cout << "\nDelete " << array << "\n";
                delete[] array;
            }

            ArrayList& operator = (const ArrayList &arraylist) {
                copy(arraylist);
            }

            void add(T elem, long index) {
                if (usedSize == totalSize) totalSize += step;

                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index, array + index + usedSize, tmp + index + 1);
                delete[] array;
                tmp[index] = elem;
                array = tmp;

                usedSize++;
            }

            void remove(long index) {
                if (usedSize == totalSize - step) totalSize -= step;
                T *tmp = new T[totalSize];
                std::copy(array, array + index, tmp);
                std::copy(array + index + 1, array + usedSize, tmp + index);
                delete[] array;
                array = tmp;

                usedSize--;
            }

            T get(long index) {
                return array[index];
            }

            long size() {
                return usedSize;
            }

            long getTotalSize() {
                return totalSize;
            }
    };

}

using namespace jpparl;
using namespace std;
int main() {
    ArrayList<ArrayList<int>> al(1);
    ArrayList<int> al2(1);
    cout << "\nAdding 256\n";
    al2.add(256,0); // al2.get(0) - 256
    cout << "\nAdding al2\n";
    al.add(al2, 0); // adds copy of a2, al.get(0) - copy of.a2
    cout << "\nRemoving 256\n";
    al2.remove(0); // removes 256 from al2, not from a1
    cout << al2.get(0) << " - must be 0      " << al.get(0).get(0) << "     - must be 256\n";
    cout << al2.size() << " - must be 0     " << al.get(0).size() << "     - must be 1\n";
}

Your `copy()` method leaks memory when called by the assignment operator. The safer way to implement a copy assignment operator is to use the [copy-swap](https://stackoverflow.com/questions/3279543/) idiom via the copy constructor — Remy Lebeau, Aug 11 '18 at 19:33
*I needed to add an assignment operator* -- Yes, but it isn't correct. There are at least 3 things wrong with it. 1) Doesn't check for self-assignment. 2) If `new` throws an exception in `copy()`, you've already changed the other member variables and no way to change them back. 3) Memory leak -- you failed to `delete[]` the previous memory. As suggested by Remy Lebeau, all of these failures can be corrected using copy-swap. — PaulMcKenzie, Aug 11 '18 at 20:07
BTW, you make a mistake of anticipating that `new[]` will be successful in your `add()` function. You change member variable values before the call to `new[]`, thus messing up the object if `new[]` throws an exception. — PaulMcKenzie, Aug 11 '18 at 20:18

Trouble with destructor using

3 Answers3