could you tell me why does this code crash?

Go To StackoverFlow.com

2

So I'm curious of the reason the following code crashes. Will appreciate help.

#include <iostream>
using namespace std;

class temp
{    
  public:

    temp(int i)
    {
        intPtr = new int(i);
    }

    ~temp()
    {
        delete intPtr;
    }

  private:
    int* intPtr;
};

void f (temp fInput)
{
    cout << "f called" << endl;
}

int main()
{
    temp x = 2;
    f(x);
    return 0;
}
2012-04-04 21:16
by user1313867
Can you define "crash" - josephthomas 2012-04-04 21:18
@ Ed S: I agree, however, sometimes what we determine to be a crash may be different than what he thinks is a crash - josephthomas 2012-04-04 21:23
@josephthomas: Yeah I figured as much, which is why I deleted my comment shortly after posting it :) We can agree on the correct use of the term... whether or not most beginners will use it correctly is another issu - Ed S. 2012-04-04 21:23
You need to manage memory correctly if you use new. Or you could just not use new and have an integer instead of a pointer to an integer - Peter Wood 2012-04-04 21:47


5

The pointer is copied when x is passed (implicit copy constructor) and the destructor is called twice (before the function returns and before main returns), thus the memory is deleted twice.

Use std::shared_ptr<int> here instead instead of a raw int pointer (assuming you want the behavior to be the same, i.e. reference the same int from both temps; otherwise, implement the copy constructor, move constructor and assignment operator yourself).

#include <memory>

class temp {    
public:
  temp(int i) : intPtr(std::make_shared<int>(i)) {

  }

private:
  std::shared_ptr<int> intPtr; // reference counting ftw
};
2012-04-04 21:20
by NoName
Or just correctly implement the class to begin with (i.e., correct copy semantics, etc. - Ed S. 2012-04-04 21:22
That's also possible, depending on the behavior you want - NoName 2012-04-04 21:23
....correct behavior is always desirable - Ed S. 2012-04-04 21:24
OP didn't mention which behavior is "correct", so I can't magically know - NoName 2012-04-04 21:28
He doesn't need to. Given the definition of the class it is incorrect to not provide a copy constructor and follow the rule of three - Ed S. 2012-04-04 21:30
With "behavior" I meant "copy the int or copy the pointer to the int?" With a shared_ptr you get the latter, and you can still assert the compiler generates a copy constructor for you - NoName 2012-04-04 21:34
But the compiler generating the copy constructor is exactly what is causing the problem. As a consumer of this class you would need to know that instances of it simply cannot be passed by value and that they need to be wrapped by something like a sahred_ptr. Horrible, horrible, lazy design - Ed S. 2012-04-04 21:38
The shared_ptr would be a member of the class, instead of the raw int pointer. That's the point of the second paragraph of my answer. Consumers don't need to worry - NoName 2012-04-04 21:41
.....and that's what I get for skimming your response :). I thought you were suggesting something completely different. Sorry, my misunderstanding. +1 for being an annoying jerk : - Ed S. 2012-04-04 21:45
I edited the answer to make my intention more clear - NoName 2012-04-04 21:47


5

The crash occurs because of the way you are passing x.

After the scope of the f function, x's destructure will be called and will delete intPtr.

However, that will delete memory that is still in scope for main. Therefor, after return 0 is called, it will try to delete memory that already exists as you are calling delete twice on the same pointer.

To fix this error, change

void f (temp fInput)

to

void f (const temp& fInput)

Alternatively, you could look into using a std::shared_ptr.

2012-04-04 21:20
by josephthomas
Adding a copy constructor and assignment operator would be a good idea as well. You could also use std::shared_ptr instead of a raw pointer, as it take care of copy semantics - Eddie Velasquez 2012-04-04 21:25
I agree, I wanted to let the poster to know a solution that could help him in a minimal effort. There is many ways this crash can be solved - josephthomas 2012-04-04 21:27


5

You are violating The Rule of Three

You maintain a pointer member and you pass a copy of the object to the function f. So, the end result is that you call delete twice on the same pointer.

2012-04-04 21:20
by Ed S.


1

The problem you're hitting here is a double delete. Because you didn't define any copy constructors here C++ is happily doing so for you. The implementation does this by performing a shallow copy of all of the contents.

f(x);  

This line works by creating a copy of x and passing it to f. At this point there are 2 temp instances which own a single intPtr member. Both instances will delete that pointer and this is likely what is causing your crash.

To work around this you can take a number of steps

  1. Use a pointer type meant for sharing like shared_ptr<T>
  2. Create an uncallable copy constructor which forces the instance to be passed by ref

An example of #2 is

class temp {
  temp(const temp& other);
  temp& operator=(const temp& other);
public:
  // Same
};

Now the f(x) line simply won't compile because it can't access the necessary copy constructor. It forces it to instead redefine f to prevent a copy.

void f(const temp& fInput) {
  ...
}
2012-04-04 21:21
by JaredPar
Ads