How do I stop memory Leaks?

Question

I keep getting error messages from the compiler saying that there is a memory leak, two 8-byte blocks and one 5-byte block. I have already deleted the *name array in the destructor. How does the destructor handle scope? I was under the impression that once the main ended that the fruit objects would be deleted as they went out of scope.

#include "Fruit.h"
#include "LeakWatcher.h"

using namespace std;
Fruit::Fruit(const Fruit &temp )
{
    name = temp.name;
    for(int i = 0; i < CODE_LEN - 1; i++)
    {
        code[i] = temp.code[i];
    }
}
void  Fruit::operator=(const Fruit &tempFruit)
{
    name = tempFruit.name;
    for(int i = 0; i < CODE_LEN; i++)
    {
        code[i] = tempFruit.code[i];
    }

}
Fruit::~Fruit()
{
    delete[] name;

}
bool  Fruit::operator==(const Fruit &tempFruit)
{
    int i = 0;
    while(name[i] != NULL && tempFruit.name[i] != NULL)  
    {
        if(name[i] != tempFruit.name[i])
            return false;
        i++;
    }
    if(name[i] != NULL || tempFruit.name[i] != NULL)
        return false;
    return true;
}
bool  Fruit::operator<(const Fruit &tempFruit)
{
    int i = 0;
    while(name[i] != NULL && tempFruit.name[i] != NULL)  
    {
        if((int)name[i] < (int)tempFruit.name[i])
            return true;
        else if((int)name[i] > (int)tempFruit.name[i])
            return false;
        i++;
    }
    if(name[i] == NULL && tempFruit.name[i] != NULL)
        return true;
    else
        return false;
}
std::ostream & operator<<(std::ostream &os, const Fruit *printFruit)
{
    os << setiosflags(ios::left) << setw(MAX_NAME_LEN) << printFruit->name << " ";
    for(int i = 0; i < CODE_LEN; i++)
    {
        os << printFruit->code[i];
    }
    os << endl;
    return os;
}

std::istream & operator>>(std::istream &is, Fruit *readFruit)
{

    string tempString;
    is >> tempString;
    int size = tempString.length();
    readFruit->name = new char[tempString.length()];
    for(int i = 0; i <= (int)tempString.length(); i++)
    {
        readFruit->name[i] = tempString[i];
    }
    readFruit->name[(int)tempString.length()] = '\0';
    for(int i =0; i < CODE_LEN; i++)
    {
        is >> readFruit->code[i];
    }
    return is;
}
void stuff()
{

}
void main()
{
    _CrtSetReportMode( _CRT_WARN, _CRTDBG_MODE_FILE ); 
   _CrtSetReportFile( _CRT_WARN, _CRTDBG_FILE_STDOUT ); 
   _CrtSetReportMode( _CRT_ERROR, _CRTDBG_MODE_FILE ); 
   _CrtSetReportFile( _CRT_ERROR, _CRTDBG_FILE_STDOUT ); 
   _CrtSetReportMode( _CRT_ASSERT, _CRTDBG_MODE_FILE ); 
   _CrtSetReportFile( _CRT_ASSERT, _CRTDBG_FILE_STDOUT );
    Fruit *fruit = new Fruit();
    Fruit *fruit1 = new Fruit();
    cin >> fruit;
    *fruit1 = *fruit;
    cout << fruit << fruit1;
    _CrtDumpMemoryLeaks();

}

H

#ifndef _FRUIT_H
#define _FRUIT_H
#include <cstring>
#include <sstream>
#include <iomanip>
#include <iostream>
enum { CODE_LEN = 4 }; 
enum { MAX_NAME_LEN = 30 };
class Fruit
{
private:
    char *name;
    char code[CODE_LEN];
public:
    Fruit(const Fruit &temp);
    Fruit(){name = NULL;};
    bool operator<(const Fruit &other);
   friend std::ostream & operator<<(std::ostream &os, const Fruit *printFruit);
    bool operator==(const Fruit &other);
   bool operator!=(const Fruit &other){return!(*this==other);};
    friend std::istream & operator>>(std::istream& is, Fruit *readFruit);
    void  Fruit::operator=(const Fruit &tempFruit);
    ~Fruit();
};
#endif

Perhaps you need to delete `fruit` and `fruit1`? Also, note that you are assigning `fruit` to `fruit1`, which means you'll never be able to free the second object. — MD Sayem Ahmed, Oct 08 '13 at 05:18
Simple: stop using `new` everywhere. If you insist on using it, then follow the [*rule of three*](http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29), and don't forget to `delete` everything you `new`. At the moment, it doesn't look like `Fruit` is safely copyable or assignable. — juanchopanza, Oct 08 '13 at 05:18
You are checking for memory leaks *before* the variables go out of scope. Of course there will be leaks shown. But then again you're not using `delete` either. — Mark Ransom, Oct 08 '13 at 05:19
I created the objects in a separate function so they went out of scope before the memory leak function. It didn't make a difference. I already tried to delete `fruit` and `fruit1` but my program actually crashed saying that heap was corrupted by writing to heap after the end of the heap buffer. name is initialized to NULL in the header. — Craig Lafferty, Oct 08 '13 at 05:24
That makes sense: as I said, `Fruit` is not safely copyable or assignable. If you do either, you will get a double delete. Read up on the rule of three (link in my first comment). — juanchopanza, Oct 08 '13 at 05:25
@CraigPatrickLafferty: You have what is called a `double delete`, you are calling `delete` on the same memory twice in your destructor. — Jesse Good, Oct 08 '13 at 05:28
Your code would be much simpler and less error prone if you used `std::string` instead of `char*`. — Marius Bancila, Oct 08 '13 at 05:29
How are they the same spot in memory? I overloaded the `"="` operator so only the members are being copied, not the pointer location. — Craig Lafferty, Oct 08 '13 at 05:31
@CraigPatrickLafferty: The line `name = tempFruit.name;` makes `name` for both objects point at the same spot in memory. — Jesse Good, Oct 08 '13 at 05:37
@CraigPatrickLafferty In your overloaded `=` operator you do copy the pointer, that's what `name = tempFruit.name;` does. So you end up with two pointers to the same location and a double delete. Use `std::string`, it's **easier**, and it's the answer to your question, 'How do I stop getting memory leaks?'. If you don't want to use std::string and you still want to avoid memory problems then write your assignment operator and copy constructor properly, they are both bugged at the moment because you are copying pointers. — john, Oct 08 '13 at 05:43

score 3 · Accepted Answer · edited May 23 '17 at 10:31

It appears that the primary source of your "leak" is the "fruit" and "fruit1" pointer - _CrtDumpMemoryLeaks() checks for memory that hasn't been freed. You need to delete the data these two point to before you call it.

That said, you have other issues.

C++ does not have automatic garbage collection. You must either track and manage your memory allocations or use classes/code that does it for you.

Consider the following code:

void iNeedALeak() {
    void* p = new char[64];
    strcpy(p, "Hello world");
    std::cout << p << std::endl;
}

This code made an allocation, and stored it in "p". Even though we used the value for a couple of functions, we never stored it. And so, it is never returned to the system.

One example of a leak waiting to happen in your code is in operator>>

std::istream & operator>>(std::istream &is, Fruit *readFruit)
{
    string tempString;
    is >> tempString;
    int size = tempString.length();
    readFruit->name = new char[tempString.length()];

Yes, in your delete operator, you delete[] from name. But that only handles the case where your code reaches ~Fruit, consider:

Fruit f;
cin >> f; // readFruit->name = new char[]..
cin >> f; // readFruit->name = new char[]...

At this point, you're no-longer storing the original value, and you didn't delete it.

Really the problem you're having is that you're using a very C-like approach. If you plan to write this in C++ you should consider using RAII, for example you could use the std::unique_ptr class.

TL;DR Don't expose raw pointers, encapsulate them behind something that ensures they get freed when they go out of scope or when they are reassigned to.

score 2 · Answer 2 · answered Oct 08 '13 at 05:20

Your fruit objects will not be deleted when they go out of scope. Instead, they will leak. Since this is the last thing your program does, there's no real consequence and the OS will reclaim the memory once the program exits. Nonetheless, it's a leak.

Why not just create them on the stack instead?

juanchopanza · Answer 3 · 2013-10-08T05:38:50.653

Rather than addressing the specific memory issues in your example, which I have already done in comments, I would avoid them entirely by using std::string and std::array. Your code would boil down to

#include <iostream>
#include <string>
#include <array>
enum { CODE_LEN = 4 }; 
class Fruit
{
private:
    std::string name;
    std::array<char,CODE_LEN> code;
public:
   bool operator<(const Fruit &other);
   bool operator==(const Fruit& other);
   bool operator!=(const Fruit& other) {return!(*this==other);}
   friend std::ostream& operator<<(std::ostream &os, const Fruit& f);
   friend std::istream& operator>>(std::istream& is, Fruit& f);
};

Note no user defined constructors, assignment operator or destructor. Some of the operators could be greatly simplified too, e.g.

bool Fruit::operator==(const Fruit& rhs)
{
  return code == rhs.code;
}

bool Fruit::operator<(const Fruit& rhs)
{
  return name < rhs.name;
}

How do I stop memory Leaks?

3 Answers3