Destructor does not destroy objects

Question

I have written a small program, but main destructor is not working properly. Here is the code of the program:

#include<iostream.h>

class citizen {
private:
    char* name;
    char* nationality;
public:             
    citizen(char* name, char* nationality) {
        this->name = name;
        this->nationality = nationality;
    }                  

    citizen(const citizen &obj) {
        name = obj.name;
        nationality = obj.nationality;
    }        

    void display() {
        cout << "Name: " << name << endl;
        cout << "Nationality: " << nationality << endl;
    }

    ~citizen() { 
        if(name){
            delete[]name;
        }
        if(nationality) {
            delete []nationality;                          
        }        
    }       
};

main() {
   citizen obj1("Ali", "Pakistani");
   obj1.display();
   { 
      citizen obj2 = obj1;                 
   }
   obj1.display();
   system("pause");
}

What I know is that in main function where I'm assigning state of obj1 to obj2, from that place both of them are now pointing to same memory area. Whereas the code citizen obj2 = obj1; is between two curly braces.

   { 
      citizen obj2 = obj1;                 
   }

So after execution of second curly brace, obj2 should destroy and also delete variables name and nationality. And when I call obj1.display(); for the second time it should display garbage on the screen.

But obj1 is still printing exact name which I have provided in constructor, even though it shouldn't be.

Please explain this behavior.

`obj1` and `obj2` hold pointers to string literals. You cannot delete these, as I pointed out in a comment to your first question. You are invoking undefined behaviour. — juanchopanza, Oct 07 '13 at 13:33
replace `char*` with `std::string` you are treating char* like it wasn't a pointer. — AndersK, Oct 07 '13 at 13:38
Your `delete`s are not paired with corresponding `new`s. That should be an indication that not all is well... Also, after you've deleted a pointer, the pointer itself is not shredded or something, so it still points to the same location in memory. Use `delete[] ptr; ptr = NULL;` to avoid that. — Mr Lister, Oct 07 '13 at 13:40

score 3 · Answer 1 · answered Oct 07 '13 at 13:33

Your delete[]s invoke undefined behavior because you're attempting to destroy string literals. Anything can happen.

Even if you allocated memory yourself, you'd still run into undefined behavior because you'd be attempting to access memory you've already deleted:

obj1.display();
{ 
   citizen obj2 = obj1;                 
}
obj1.display();  // ILLEGAL

Because you didn't define an assignment operator, the compiler-generated one will be used, which just assigns the pointers to the same memory - memory which you destroy and then attempt to access.

score 2 · Answer 2 · edited May 23 '17 at 12:06

Your copy constructor just copies the pointers (as the implicit one would do, if you hadn't provided your own), meaning that both objects will try to delete the same arrays. Additionally, you're setting the pointers to point to string literals, which must not be deleted at all; you must only delete objects that you created with new. The simple solution is to delegate memory management to a class designed to do that correctly:

std::string name;
std::string nationality;

Now you don't need to bother with your own destructor, copy constructor or copy-assignment operator; and as a bonus, your class will also be correctly movable in C++11.

If you enjoy dealing with memory issues yourself, then you'll need your constructors to allocate new buffers and copy the contents across. Be careful with exception safety, since you're trying to juggle two separate dynamic resources in one class, which is invariably a recipe for errors. You'll also need a copy-assignment operator (per the Rule of Three), and for efficiency you might also consider a move constructor and move-assignment operator.

Also, it might be worth updating to one of this century's versions of the language. <iostream.h> hasn't been a standard header for about fifteen years.

score 2 · Answer 3 · answered Oct 07 '13 at 13:38

2

This code is faulty.

     if(name){
              delete[]name;
     }
     if(nationality){                          
              delete []nationality;                          
     }

You are deleting, something which you have not allocated on heap, using the new operator.

answered Oct 07 '13 at 13:38

Barath Ravikumar

5,658
3
23
39

1

+1. If you didn't allocate something, don't delete it. If you did allocate something, you eventually have to delete it. Once. If you don't know, you had better know if you are using pointers. Even better, don't use pointers. Use types such as `std::string` that do all of this knowin' for you. – David Hammen Oct 07 '13 at 13:50

score 0 · Answer 4 · answered Oct 07 '13 at 13:37

0

Others have pointed out the string related error but I think you're making a much more fundamental mistake: delete does not destroy things*; it merely frees up the memory for reuse and calls the relevant destructor. This means that its often entirely possible to use deleted objects after the delete operation without getting garbage back.

*- On some implementations, in __DEBUG mode it will stamp on released memory to allow you to spot these errors but this isn't part of the standard.

answered Oct 07 '13 at 13:37

Jack Aidley

19,439
7
43
70

"This means that its often entirely possible to use deleted objects after the delete" - not without running into undefined behavior. – Luchian Grigore Oct 07 '13 at 13:49
@LuchianGrigore: Well, duh. – Jack Aidley Oct 08 '13 at 07:58
Yeah, duh. But for someone who asks this question, it isn't that obvious. In fact, that statement can be outright confusing. – Luchian Grigore Oct 08 '13 at 08:10

score 0 · Answer 5 · answered Oct 07 '13 at 13:49

you should only delete block of memory that was dynamically allocated in the class using new operator

citizen(char* aname, char* anationality){

     size_t nameSize = strlen(aname)
     size_t nationalitySize = strlen(anationality)

     this->name = new char[nameSize];
     this->nationality = new char[nationalitySize];

     strcpy(this->name, aname, nameSize);
     this->name[nameSize ] = NULL;

     strcpy(this->nationality , anationality , nationalitySize);
     this->nationality [nationalitySize] = NULL;
}

if you create a memory in constructor, then you delete them in destructor. and because you have some assignment, then you should implement a copy constructor, WHICH create a memory and copy the data on the right side of the = operator

final thought, you are better off using a string object if you cant deal with pointers to avoid memory leaks

score 0 · Answer 6 · edited May 23 '17 at 11:50

It's pure luck that your second display call works.

As Jack Aidley points out, delete does not literally delete the values, just marks the memory region as usable. Thus, if no other application allocates and modifies that freed region, the previous values may stay there. Furthermore, after the deletion you still keep the pointers to that addresses.

In summary, you're accessing the same memory region having old values in it. But it's just luck because that region didn't modified by anybody.

To prevent these kinds of errors, you should always assign NULL to the unused pointers so that next time you get Access Violation error when you try to access them. Some compilers (such as MSVC) rewrite the freed memory with a signature value (such as 0xDDDDDD) so that during debugging you can catch the problems easily. Check this answer for details

Finally, delete should match to new otherwise the behavior is undefined so it's again just luck. I can't even run your application and display results. It keeps crashing and giving memory errors.

thanx bro i have replace this code of constructor citizen(char* name, char* nationality) { this->name = name; this->nationality = nationality; } with this code citizen(char* name, char* nationality){ this->name = new char[strlen(name)+1]; strcpy(this->name, name); this->nationality = new char[strlen(nationality)+1]; strcpy(this->nationality, nationality); } and finally it works fine. thanks — , Oct 07 '13 at 15:03
yep, this looks better :) by the way, you're not experimenting for this but don't forget to put `name = NULL` and `nationality = NULL` into the destructor to prevent illegal accesses. Because accessing to freed memory space is considered as illegal. — SylvanBlack, Oct 08 '13 at 11:04

score 0 · Accepted Answer · answered Mar 29 '15 at 16:29

Thanks everybody. I have replace this code of constructor

citizen(char* name, char* nationality) {
    this->name = name;
    this->nationality = nationality;
}

with this code

citizen(char* name, char* nationality){ 
   this->name = new char[strlen(name)+1]; 
   strcpy(this->name, name); 
   this->nationality = new char[strlen(nationality)+1]; 
    strcpy(this->nationality, nationality); 
}

and finally it works fine. thanks

Destructor does not destroy objects

7 Answers7