Delete[] causing crash in C++

Question

I am working with the same code from this code from my previous SO post but I have made several changes. My problem is that I have a dynamic array that I call delete[] on inside of my operator overload for the copy constructor and I get the error below.

Exception thrown at 0x0F7063BB (ucrtbased.dll) in Lab3.exe: 0xC0000005: Access violation reading location 0xCCCCCCBC.

If there is a handler for this exception, the program may be safely continued.

Can anyone help me understand why? I checked the related questions but there are different errors than from what I'm seeing and I haven't found a result in my google search. I am using C++ 11 on visual studio 2015.

#include "ListArray.h"

template < typename DataType >
List<DataType>::List ( int maxNumber )
{
    //maxSize = MAX_LIST_SIZE; maybe???
    maxSize = maxNumber;
    dataItems = new DataType[maxSize];
    size = maxSize - 1;
    cursor = 0; // sets cursor to the first value in list
    for (; cursor <= size; cursor++)
        dataItems[cursor] = 1;
    cursor = 0;
}

template < typename DataType >
List<DataType>::List ( const List& source )
{
    *this = source; // should be handled by copy constructor
}

template < typename DataType >
List<DataType>& List<DataType>::operator= ( const List& source )
{
    if (this != &source)
    {
        maxSize = source.maxSize;
        size = source.size;
        cursor = source.cursor;
        delete []dataItems; // clears dataItems, weird run-time error here. Why? 
        dataItems = new DataType[size];
        for (int count = 0; count < size; count++)
            dataItems[count] = source.dataItems[count];
    }
    else
        // do nothing, they are the same so no copy is made
        return *this;
}

template < typename DataType >
List<DataType>::~List ()
{
    maxSize = 0;
    size = 0;
    cursor = -1;
    delete[] dataItems;
}

Edit: I initially posted several other incomplete functions also a part of the program I am trying to build. I meant only to include the ones that I know are generating my problem. My apologies for the bad post.

`if (cursor = size - 1)` are you sure this condition with assignment is correct? — MikeCAT, Jun 16 '16 at 02:05
The error seems because `dataItems` is used before being initialized. Try initializing it before `*this = source;` — MikeCAT, Jun 16 '16 at 02:07
No but my code doesn't even get that far down. It doesn't run past the operator= function because I am testing them in order — Callat, Jun 16 '16 at 02:07
Unfortunately, stackoverflow.com is not a service for debugging large chunks of code. It's OK if a [mcve] is provided; but for pages and pages of source, the answer is going to be: learn how to use a debugger. — Sam Varshavchik, Jun 16 '16 at 02:08
Expressing copy constructor in terms of assignment is not a good approach in general. In particular it blows up (so to speak) in derived classes. Instead express copy assignment in terms of copy construction, via the copy-and-swap idiom. — Cheers and hth. - Alf, Jun 16 '16 at 02:09
@SamVarshavchik please refer to my edit I am only asking for help on the first 3 functions. Default constructor, copy constructor(with assignment operator), and destructor. I made a small mistake in uploading and I am still editing. — Callat, Jun 16 '16 at 02:10
Please reduce the problem to a **minimal but complete** example that readers can try by copying and pasting the code. Voted to close as lacking reproducible example. — Cheers and hth. - Alf, Jun 16 '16 at 02:11
Write the copy constructor as if there is no assignment operator. When you get that done, then write the assignment operator in terms of the copy constructor using copy / swap. The way you're doing things now is topsy-turvy. — PaulMcKenzie, Jun 16 '16 at 02:16
@Alf, Paul: There's an efficiency loss, but it should not "blow up". If it does, then `Derived x(initializer); Derived y; y = x;` would also blow up. — Ben Voigt, Jun 16 '16 at 02:19
@Hikari: No, my friend Alf said that calling assignment operator from copy constructor "blows up" in derived classes, and I'm responding to his comment. — Ben Voigt, Jun 16 '16 at 02:22
@Hikari What if during assignment, `new[]` throws an exception? You've messed up your object since you changed your member variables (and deleted the data) before you issued the `new[]` call. That's why you should really consider doing copy / swap so that things like this don't happen. You've just written code with all of the flaws that are pointed out [here](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) — PaulMcKenzie, Jun 16 '16 at 02:26
Implementing copy constructor in terms of assignment operator is not going to fly. It is done the other way around! — SergeyA, Jun 16 '16 at 02:34
Ok so what is this error then? I'd like to know for future reference. Link? Definition? — Callat, Jun 16 '16 at 02:36
@SergeyA: It flies just fine... assuming the copy assignment operator is correctly written. Paul is pointing out (correctly) how hard that can be. — Ben Voigt, Jun 16 '16 at 02:36
@BenVoigt, not without a lot and unnecessary efforts. For example, it means swap idiom is no longer applicable. How are you ensuring exception safety in assignment than? Than, there is a check for equality. Never going to happen for copy ctor, just performance hit for nothing. And I can continue. — SergeyA, Jun 16 '16 at 02:39
@SergeyA: It's just moving the problem around. Writing a perfectly safe copy constructor from scratch (no use of assignment operator) is only slightly easier than writing a perfectly safe assignment operator from scratch (no copy+swap). All those things Paul pointed out can go wrong -- can also go wrong when writing the copy constructor from scratch. The compiler can optimize out the identity check if it inlines the assignment operator body into the copy constructor. And it can perhaps optimize other things like both pre-initializing the members and testing to see if they need to be freed — Ben Voigt, Jun 16 '16 at 02:41
@BenVoigt, no, not really - with proper type of members safe copy constructor is a piece of cake. — SergeyA, Jun 16 '16 at 02:42
@SergeyA: In such cases, safe copy assignment operator is also a piece of cake (hint: copy+swap individual members, instead of the whole class) — Ben Voigt, Jun 16 '16 at 02:44
@BenVoigt: The derived class copy constructor can't very well call the base copy constructor (in member initializer list) and then its own assignment operator, unless it wants the base class assignment called twice. So there are two possibilities: (1) call the base default constructor and then its own assignment operator, or (2) call the base copy constructor, default init derived class items and then assign the derived class items. It's approach (2) that blows up in code size, with ever more duplication of code, and I've worked on such code. Sorry, I should have noted that (1) is non-fatal. — Cheers and hth. - Alf, Jun 16 '16 at 03:28
@Alf: Right. The pattern is (default-construct, then assign). The derived copy-constructor ought to be delegating to its own default constructor, and C++11 now makes this possible in a way that prevents it from directly calling any base constructors at all (the derived default constructor will call the base default constructor, which is the obvious and only reasonable choice). So the pain point is gone since C++11. — Ben Voigt, Jun 16 '16 at 04:15

score 2 · Answer 1 · answered Jun 16 '16 at 02:15

2

The copy constructor does not initialize anything in the new instance of the class. All it does is call the assignment operator.

The assignment operator executes:

delete []dataItems;

Since dataItems has not been initialized, this results in undefined behavior, and an immediate crash.

answered Jun 16 '16 at 02:15

Sam Varshavchik

114,536
5
94
148

I don't understand what you mean by nothing is initialized. If my other constructor exists above then doesn't `*this` get instantiated then passed to the copy to be reset? – Callat Jun 16 '16 at 02:19
@Hikari: Only subobject constructors get implicitly called. If you want one of your constructors (the copy constructor) to call another one (the default constructor), you have to tell the compiler to do it. Right now your copy constructor starts with raw memory whose contents are random, then immediately calls `operator=()` – Ben Voigt Jun 16 '16 at 02:20
Can you provide an example of how I would do that? Would this be an initialization list? – Callat Jun 16 '16 at 02:22
When a new object gets instantiated, only a single constructor gets called. If a copy constructor gets called, then only the copy constructor gets called. If the default constructor gets called, only the default constructor gets called. C++1x introduced delegating constructors, which would be an option here. – Sam Varshavchik Jun 16 '16 at 02:27

score 2 · Accepted Answer · edited May 23 '17 at 11:58

If you're going to use the assignment operator to make your copies, you need an empty object to start with, otherwise operator= will fail horribly when it tries to clean up the old contents before replacing them (as Sam explained in his answer and MikeCAT mentioned in his comment -- give them upvotes -- and what Dieter told you on your earlier question).

Since C++11, constructor chaining aka constructor delegation is possible, like this:

template < typename DataType >
List<DataType>::List ( const List& source )
    : List() /* default construct object before proceeding */
{
    *this = source; // should be handled by copy constructor
}

another option is to make the copy constructor responsible for creating a default object itself, and this is what would be needed in C++98 and C++03:

template < typename DataType >
List<DataType>::List ( const List& source )
    : maxSize(0), dataItems(NULL), size(0) // initialize members
{
    *this = source; // should be handled by copy constructor
}

But this is duplicating the function of the default constructor, so as long as your compiler has support for the C++11 feature, prefer to call the existing one.

The details of constructor delegation... get complicated. Among other things, normally any exception thrown from inside an object constructor prevents the object from ever existing, and the destructor is not called. When delegation is used, then the object becomes live when any constructor completes, and an exception inside a wrapper constructor will face an object that's already alive and call the destructor for you. This behavior may or may not be desirable, but it's something to watch out for because exception-safety is important in modern C++.

THIS! Implementing this fixed my code completely thank you sir! — Callat, Jun 16 '16 at 03:37

Delete[] causing crash in C++

2 Answers2