2

I have a small C++ program where I create two objects of a Person class. This class has char *m_szFirstName and char *m_szLastName for data.

I then I assign one object to another, causing both object's data members to point to same location.
In the destructor, I delete what memory I had allocated for the first object and assign NULL values to the pointers. Something like this.

if (m_szFirstName!= NULL)
{
    delete [] m_szFirstName;
    m_szFirstName = NULL;
}

Then when I go to delete memory for the second object, the check for NULL does not work and when I delete memory, I get a crash. From debugger, it show that my pointer is not NULL. It has 0xfeee.

I am aware that memory was already deleted before and should not be delete. However, I don't know how to check whether I should delete memory or not.

Sam
  • 7,252
  • 16
  • 46
  • 65
Saleem Yusuf
  • 21
  • 1
  • Can you show some code ? – cnicutar Feb 17 '12 at 07:15
  • When you copy your `Person` instance, the second object has **its own** members. That is, it has its own `m_szFirstName` whose value was copied from the original instance. Note that only the pointer is copied, **not the pointed memory**. So when you set the first instance to `NULL` after deleting it, the second one keeps its value and points to "deleted" memory. – ereOn Feb 17 '12 at 07:34
  • the code is too big to attach in here. I want like to demonstrate you cannot assing one object (with char * data) to another unless you ovride the assignment operator and do a copy in it. Otherwise the two object's data will point to same memory. When the first object is deleted, second object data will have corrupted data. – Saleem Yusuf Feb 19 '12 at 18:10

6 Answers6

4

Reason for Crash:
You should follow the Rule of Three to avoid this problem of dangling pointers.

If you need to explicitly declare either the destructor, copy constructor or copy assignment operator yourself, you probably need to explicitly declare all three of them.

In your case You do not define a copy assignment operator thus leading to shallow copy of the pointer.

Suggested Solution:

If you can use std::string instead of char * just simply use std::string, it has the first and foremost preference over any kind of silly pointer stuff.
You can avoid all the funky pointer stuff by using std::string.

If you can't read on and the following suggestion applies to any class pointer member in general.

Note that the ideal solution here is to not use raw pointers at all, Whenever you use raw pointers you are forced to manually manage the resources acquired by them, it is always difficult and error prone to manually manage resources.So the onus is to avoid it.

To do so, You should use a Smart pointer which will manage the dynamic memory of the pointer implicitly.Using a smart pointer will ensure that the dynamic memory is implicitly released after usage & you do not have to manually manage it.

The scenario you have is the very reason that in C++ you should rely on RAII rather than manual resource management & using a Smart pointer is the way to go about in your case.

Caveat:
Note that I restrained myself from suggesting which smart pointer to use because the choice rather depends on the ownership and lifetime of the elements involved, which is not clear from the data provided in the Question.So I will suggest reading,

Which kind of pointer do I use when?

to make your choice of the smart pointer to use.

Community
  • 1
  • 1
Alok Save
  • 202,538
  • 53
  • 430
  • 533
  • Note: When RAII is being used, you still should follow the [Law Of The Big Two] http://www.artima.com/cppsource/bigtwo.html – Arafangion Feb 17 '12 at 07:32
  • Good answer. However, in his case, it is very likely that what he really needs, is a `std::string` to store its firstnames and lastnames. – ereOn Feb 17 '12 at 07:37
  • @ereOn: *Likely* Yes, *Surely* We don't know, Anyways the answer holds good in general for any pointer members as such. Ofcourse, If One can use `std::string` it has the first and foremost preference over any kind of silly pointer stuff. I see lot of Assignments related Q's pushing for solutions not using `std::string` which is kind of silly, I am assuming this one of those cases.Anyways I will edit the answer to reflect this.Thanks. – Alok Save Feb 17 '12 at 07:39
  • @Als: You are absolutely right: at this point we can only guess. Very good and complete answer anyway now: here is my +1. – ereOn Feb 17 '12 at 08:16
  • Thank you for some good suggestions. What I am looking for is how to tell if pointer is valid or not before deletion. If I delete a ponter, assign it a null value and then delete again, it should not crash. Also the check for NULL fails the second time, because the pointer was deleted and it no longer a valid pointer. – Saleem Yusuf Feb 19 '12 at 18:25
1

With

if (m_szFirstName!= NULL)
{ 
  delete [] m_szFirstName;
  m_szFirstName = NULL;
}

You only set m_szFirstName to point to NULL, not m_szLastName. This means you have to have some way to keep track of the fact they point to the same location. Is there a reason they point to the same location? Could you copy the name instead of pointing the pointers to the same place?

If you need the two pointers to shared the same data, I would have a look at std::tr1::shared_ptr, which will solve this issue for you by keeping track of the number of references and deleting when the number of references reaches 0.

martiert
  • 741
  • 4
  • 13
0

Don't delete it again, if (m_szFirstName == m_szLastName). But this will give you a memory leak (when you assign one pointer to other).

dbrank0
  • 9,026
  • 2
  • 37
  • 55
0

When you have two pointers pointing to the same location (after you assigned the first one to the second one), you have two pointers pointing at the same address. Deleting one frees the memory pointed to by both of them. But setting one to NULL doesn't alter the other pointer. The same happens if you have two integers, for example.

int a = 3;
int b = a;

Now if you run

a = 0;

the value of b doesn't change. The same way your second pointer doesn't change when you alter the first one (but when you alter the memory pointed to by either pointer, you can see the effect through the other as well).

Peter
  • 531
  • 4
  • 4
0

Your problem is a classic C/C++ problem which is known as "Dangling Pointer" problem. Dereferencing the dangling pointer has resulted in the crash. The problem is about reference counting. Once you assign the same memory to the second pointer then the reference count should be 2. So if you delete one pointer reference count should become 1 and your memory should not be deallocated or freed unless count is 0. At 0 it can be garbage collected.

Now there are good answers above to solve your problem. Since you are using C++ so you can use something like an auto_ptr (OR shared_ptr). They provide what I had mentioned above, the reference counting stuff and even you need not worry about deleting or freeing your objects, these classes would take care. They work on simething known as RAII pattern where a destructor is auto-magically called when the object on the stack goes out of scope.

Yavar
  • 11,883
  • 5
  • 32
  • 63
0

Just stop setting pointers to NULL when you have deleted the object. As you can see, it just leads to pain. You cannot assume that because the pointer is not-NULL, it has not been deleted already.

You can use any sensible pattern you want to avoid this problem. For example, Boost's shared_ptr is a great choice.

David Schwartz
  • 179,497
  • 17
  • 214
  • 278
  • Setting pointer to nullptr or NULL is the proper thing to do after deleting it so that if you delete the same pointer again, nothing should happen. In my case, for some stupid reason, it crashes. Thank you anyway for your suggestion. – Saleem Yusuf Feb 23 '12 at 22:44
  • @SaleemYusuf It doesn't stop you from causing a crash if you delete the same pointer again, as you saw. All it does is make you think that if a pointer variable holds a value that's non-NULL, that means it's safe to access, which is **not** true. So it's a very bad habit to get into. Rather than setting the pointer to NULL, just don't access it. (Or, better yet, use a sensible pointer class.) – David Schwartz Feb 23 '12 at 22:48