Fixing memory leak

Apr 26, 2018 at 10:01pm
I'm having trouble fixing a memory leak in my code. Based on using valgrind, the memory leak is occurring on line 6 in the Student constructor, but I'm not sure how to delete the new array I made on the heap because I tried to delete in the destructor, but it isn't working. Any help is appreciated.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
  #include "student.h"
  #include <string>
  #include <cstring>
  #include <sstream>
  Student::Student(const char * const name, int perm) {
        this->name = new char[strlen(name) +1];
        strcpy(this->name, name);
        this->perm = perm;
  }

  int Student::getPerm() const {
        return perm;
  }

  const char * const Student::getName() const {
        return name;
  }

  void Student::setPerm(const int permNumber) {
        this->perm = permNumber;
  }

  void Student::setName(const char * const name) {
        this->name = new char[strlen(name)+1];
        strcpy(this->name, name);
  }


  Student::Student(const Student &orig) {
        this->setName(orig.getName());
        this->setPerm(orig.getPerm());
  }

  Student::~Student() {
        delete [] name;
Apr 26, 2018 at 10:07pm
What happens on line 24?


That is not all. You should read: http://en.cppreference.com/w/cpp/language/rule_of_three



PS. There is a difference between initialization and the change of existing objects.
1
2
3
4
5
6
7
8
9
10
11
12
13
Student::Student(const char * const name, int perm)
 : name( new char[strlen(name) +1] ),
   perm( perm )
{
  strcpy( this->name, name );
}
// vs
Student::Student(const char * const name, int perm)
{
  this->name = new char[strlen(name) +1];
  strcpy(this->name, name);
  this->perm = perm;
}

Prefer the member initialization list syntax
Last edited on Apr 26, 2018 at 10:10pm
Apr 26, 2018 at 10:16pm
Oh sorry I forgot to include the rest of the code which has the overloaded assignment operator

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
Student & Student::operator=(const Student &right) {
  // The next two lines are standard, and you should keep them.
  // They avoid problems with self-assignment where you might free up
  // memory before you copy from it.  (e.g. x = x)

        if (&right == this)
                return (*this);

  // TODO... Here is where there is code missing that you need to
  // fill in...
        this->setName(right.getName());
        this->setPerm(right.getPerm());



  // KEEP THE CODE BELOW THIS LINE
  // Overloaded = should end with this line, despite what the textbook says.
        return (*this);

}

std::string Student::toString() const {
        std::ostringstream oss;

        oss << "["
            << this->getName() << ","
            << this->getPerm() << "]";
        return oss.str();

}


On line 24, it takes in a name and sets the Student's private variable of "name" to be what is taken in.
Apr 26, 2018 at 10:49pm
Lets do:
1
2
3
4
int main() {
  Student snafu( "John", 42 );
  snafu.setName( "Dolly");
}

What does happen there? Calls to constructor, setName, and destructor.

What of interest does happen in them?
1
2
3
snafu.name = new char[strlen("John") +1]; // line 6
snafu.name = new char[strlen("Dolly") +1]; // line 24
delete [] snafu.name;

The constructor allocates a block of memory. Lets call it 'foo'.
The setName allocates a block of memory. Lets call it 'bar'.
The destructor deallocates a block. Which block? The bar.

What did happen to the 'foo'?
Apr 26, 2018 at 11:11pm
So the new char[] in the constructor wouldn't be affected by the destructor. If I were to change the constructor to be in the member initialization syntax, would this change anything?
Apr 27, 2018 at 5:04am
No, that won't, but it is still recommended to do, because in some cases you absolutely have to use member initialization list. Why not make it a habit from day one?

A real issue is here:
1
2
3
4
5
6
7
void Student::setName(const char * const name)
{
  // this->name points to some memory block 'foo'
  this->name = new char[strlen(name)+1];
  // nothing points to memory block 'foo' any more
  strcpy( this->name, name );
}

The previously allocated block "foo" leaks every time you call setName().

You should deallocate in setName().


However, what if strlen(name) <= strlen(this->name)?
Do you still have to deallocate&allocate?
Apr 27, 2018 at 5:49pm
How would I deallocate in setName()?
1
2
3
4
5
6
7
void Student::setName(const char * const name)
{
  
  this->name = new char[strlen(name)+1];
  delete [] name;
  strcpy( this->name, name );
}


This is resulting in a seg fault when I try to test the code.
Apr 27, 2018 at 6:47pm
1
2
3
4
5
6
7
8
9
10
11
12
void Student::setName(const char * const name)
{
  // this->name points to some memory block 'foo'
  this->name = new char[strlen(name)+1];
  // this->name points to different memory block 'bar'
  // the 'foo' has already leaked

  delete [] name; // attempts to deallocate argument the 'name', not the this->name
  // if you try 'delete [] this->name' here, you deallocate the 'bar'

  strcpy( this->name, name );
}

Think also the order of operations.
Apr 29, 2018 at 11:13pm
Thank you for all the help. I was able to fix the memory leaks.
Topic archived. No new replies allowed.