Buffer Overrun with delete []

Question

I tried to search same questions, but not one helped me. When I run program I get the "A Buffer Overrun has occurred..." error.

Constr:

Player(char* n)
{
   length = strlen(n);
   name = new char[length+1];

   for(unsigned int i(0); i < length; i++)
       name[i] = n[i];

   name[length] = '\0';
}

Destr:

~Player(void)
{
   delete [] name;
}

I've NULL terminated string and don't get out of bounds, what is problem?

Does the class follow the [Rule of Three](http://stackoverflow.com/questions/4172722)? If not, there's a good chance you're deleting the same buffer twice. Use `std::string` to manage the dynamic memory correctly for you, unless you particularly want to practice your pointer-juggling skills. — Mike Seymour, Jul 07 '14 at 11:33
How do you know it is this particular code that causes the problem? — Jon, Jul 07 '14 at 11:35
Related: [Rule of five](http://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c11) — Deduplicator, Jul 07 '14 at 11:47

score 1 · Accepted Answer · edited May 23 '17 at 12:29

There's no obvious error in the code you've posted, but trying to manage dynamic memory by juggling raw pointers will almost inevitably lead to errors like this.

Perhaps you haven't correctly implemented or deleted the copy constructor and copy-assignment operator, per the Rule of Three. In that case, copying a Player object will give two objects with pointers to the same array; both of them will try to delete that array, giving undefined behaviour.

The simplest solution is to manage your string with a class designed for managing strings. Change the type of name to std::string, and then the constructor can simply be something like

explicit Player(std::string const & n) : name(n) {}

and there's no need to declare a destructor (or move/copy constructor/assignment operators) at all.

score 0 · Answer 2 · answered Jul 07 '14 at 12:14

So... a solution by using an std::string has been provided, but let me give another solution, keeping your member variables intact.

The problem is this. Suppose you have this code somewhere:

Player p1("Bob"); // Okay
Player p2("Annie"); // Okay
p2 = p1; // Oops! (1)
Player p3(p1); // Oops! (2)

At (1), the method Player& Player::operator=(const Player&) is called. Since you didn't provide one, the compiler generates one for you. When it does, it simply assumes that it may copy over all member variables. In this case, it copies over Player::name and Player::length. So, we have p1.name == p2.name. Now when the destructor of p2 is called, the allocated memory pointed to by p2.name is deleted. Then when the destructor of p1 is called, the same memory will be deleted (since p1.name == p2.name)! That's illegal.

To fix this, you can write an assignment operator yourself.

Player& Player::operator = (const Player& other)
{
    // Are we the same object?
    if (this == &other) return *this;

    // Delete the memory. So call the destructor.
    this->~Player();

    // Make room for the new name.
    length = other.length;
    name = new char[length + 1];

    // Copy it over.
    for (unsigned int i = 0; i < length; ++i) name[i] = other.name[i];
    name[length] = '\0';

    // All done!
    return *this;
}

At (2), the same problem occurs. You do not have a copy constructor, so the compiler generates one for you. It will also assume that it may copy over all the member variables, so when the destructors get called, they'll try to delete the same memory again. To fix this, also write a copy constructor:

Player::Player(const Player& other)
{
    if (this == &other) return;
    length = other.length;
    name = new char[length + 1];
    for (unsigned int i = 0; i < length; ++i) name[i] = other.name[i];
}

At the end of the day you should use an std::string though.

Thanks! I was too close to this problem, but i did not do this cuz read about it 10min ago ;) suddenly found that I didn't make a mistake with 'delete []'. I'm sorry for incorrect question, but these answers solved the problem i didn't know about. Thanks again! — pushandpop, Jul 07 '14 at 12:32
This is not the best example of implementing copy semantics. In particular, don't call the destructor - that gives undefined behaviour (even though you'll probably get away with it here). I'd hope for reasonable exception-safety; this leaves the assignment target with a dangling pointer if memory allocation fails. Consider the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543) for safe assignment using the copy-constructor, rather than hand-coding both functions. There's no need to check for the same object in the constructor, since you're initialising a new object. — Mike Seymour, Jul 07 '14 at 16:04

Buffer Overrun with delete []

2 Answers2