How to solve problems with Memory Leaks in C++ code

Question

This is the code that I'm using to setName

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

and this is my deletor

Student::~Student() {
    perm = 0;
    delete[] this->name;
}

but when I run valgrind, I get

13 bytes in 1 blocks are definitely lost in loss record 1 of 1
==1786==    at 0x4C2FBC3: operator new[](unsigned long) 
(vg_replace_malloc.c:433)
==1786==    by 0x402167: Student::setName(char const*) 
(student.cpp:25)
==1786==    by 0x4020F1: Student::Student(char const*, int) 
(student.cpp:7)
==1786==    by 0x401A73: main (testStudentRoll01.cpp:11)

If you are using C++ why aren't you using `std::string`? It saves all the problems of having to manually `new[]`, `delete[]`, `memcpy`, etc. — Cory Kramer, Jan 23 '19 at 19:43
You are not `delete[]`ing the old memory that `name` previously points to before reassigning `name` to point at new memory. That is your leak. Also, make sure you are following the [Rule of 3/5/0](https://en.cppreference.com/w/cpp/language/rule_of_three) in your class to avoid other similar leaks. — Remy Lebeau, Jan 23 '19 at 19:44
You might be calling `setName()` multiple times but it gets deleted only once since its in destructor. — zar, Jan 23 '19 at 19:44
@VikramPasupathy the same way you do in your destructor - using `delete[]` — Remy Lebeau, Jan 23 '19 at 19:48
@VikramPasupathy -- *I am forced to use char[]* -- Create your own string class that does basic things, and just use it. You have most of the code now to do this, it's just that you've got it tangled up in a `Student` class. — PaulMcKenzie, Jan 23 '19 at 20:35

Remy Lebeau · Accepted Answer · 2019-01-23T19:59:21.347

2

You are not delete[]ing the memory that name already points to before reassigning it to point at newly allocated memory. That is your leak.

Try this instead:

void Student::setName(const char * const name)
{
    delete[] this->name; // <-- add this
    this->name = NULL; // in case new[] below throws an exception...

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

Or better, use the copy-and-swap idiom to provide better exception safety:

#include <algorithm>

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

void Student::setName(const char * const name)
{
    Student temp(name);
    std::swap(this->name, temp.name);
}

Also, make sure you are following the Rule of 3/5/0 in your class to avoid a similar leak in your copy assignment operator (assuming you have even implemented one - the default generated one WILL leak in this situation).

A better solution is to simply use std::string instead of char* and let it handle memory management for you.

edited Jan 23 '19 at 19:59

answered Jan 23 '19 at 19:48

Remy Lebeau

555,201
31
458
770

1

Obviously we're following the good practices so... `nullptr`? – Fureeish Jan 23 '19 at 19:50
Conditional jump or move depends on uninitialised value(s) – v.p Jan 23 '19 at 19:50
@Remy Lebeau it's giving me an error based on conditional jumps – v.p Jan 23 '19 at 19:51
1

@VikramPasupathy that means you copy-pasted the code incorrectly. No such thing should be generated by code that Remy proposed. – Fureeish Jan 23 '19 at 19:52
1

@Fureeish since the OP is resorting to old-school C `strcpy()`, good chance he's not using C++11, either. Which is why I opted for `NULL` instead of `nullptr`. But, I added a second example that does not rely on either. – Remy Lebeau Jan 23 '19 at 19:54
void Student::setName(const char * const name) { 24 delete[] this->name; 25 this->name = NULL; 26 27 this->name = new char[strlen(name)+1]; 28 strcpy(this->name,name); 29 } – v.p Jan 23 '19 at 19:55
Conditional jump or move depends on uninitialised value(s) ==3409== at 0x402077: Student::setName(char const*) (student.cpp:24) ==3409== by 0x4020FF: Student::Student(Student const&) (student.cpp:33) ==3409== by 0x4022DE: StudentRoll::insertAtTail(Student const&) (studentRoll.cpp:12) ==3409== by 0x401B71: main (testStudentRoll02.cpp:27) – v.p Jan 23 '19 at 19:55
^ My error that comes when I use your code @RemyLebeau – v.p Jan 23 '19 at 19:56
@VikramPasupathy There are no conditional jumps or uninitialized values in the code I showed. But, do make sure you have initialized `this->name` to something meaningful (even null) before calling `setName()`. Looks like you are calling `setName()` in your copy constructor, you probably forgot to initialize `name` first, so it would have an *indeterminate* value in `setName()` and fail on the `delete[]`. – Remy Lebeau Jan 23 '19 at 19:57

How to solve problems with Memory Leaks in C++ code

1 Answers1