stset stset - 9 months ago 50
C++ Question

valgrind finding leaks that are hard to find

This is part of a program. When i compile it with g++, it compiles and works fine, but valgrind says that there are definitely lost: 256 bytes in 8 blocks in the leak summary. I narrowed dow the part where the memory leak happens. I tried different things to fix it but valgrind keep finding ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0). the main leak is happening when the buildTree function is called. how can i fix it?

#include <fstream>
#include <iostream>
using namespace std;

const int ARRAYSIZE = 100;

class NodeData {
friend ostream & operator<<(ostream &, const NodeData &);
public:
NodeData(); // default constructor, data is set to an empty string
~NodeData();
NodeData(const string &); // data is set equal to parameter
NodeData(const NodeData &);
bool setData(istream&);

bool operator==(const NodeData &) const;
bool operator!=(const NodeData &) const;
bool operator<(const NodeData &) const;
bool operator>(const NodeData &) const;
private:
string data;
};

NodeData::NodeData() { data = ""; } // default

NodeData::~NodeData() { } // needed so strings are deleted properly

NodeData::NodeData(const NodeData& nd) { data = nd.data; } // copy

NodeData::NodeData(const string& s) { data = s; }

bool NodeData::operator==(const NodeData& rhs) const {
return data == rhs.data;
}

bool NodeData::operator!=(const NodeData& rhs) const {
return data != rhs.data;
}

//------------------------ operator<,>,<=,>= ---------------------------------
bool NodeData::operator<(const NodeData& rhs) const {
return data < rhs.data;
}

bool NodeData::operator>(const NodeData& rhs) const {
return data > rhs.data;
}

bool NodeData::setData(istream& infile) {
getline(infile, data);
return !infile.eof(); // eof function is true when eof char is read
}

//-------------------------- operator<< --------------------------------------
ostream& operator<<(ostream& output, const NodeData& nd) {
output << nd.data;
return output;
}

class BinTree
{
public:
BinTree(); // default constructor
BinTree(const BinTree&); // copy constructor
~BinTree();
void makeEmpty();
bool insert(NodeData*);
private:
struct Node
{
NodeData* data; // pointer to data object
Node* left; // left subtree pointer
Node* right; // right subtree pointer
};
Node* root; // root of the tree
void makeEmptyHelper(Node*&);
bool insertHelper(Node*&, NodeData*);
};


BinTree::BinTree()
{
root = NULL;
}

BinTree::BinTree(const BinTree& bt)
{
this->root = NULL;
*this = bt;
}

BinTree::~BinTree()
{
makeEmpty();
}

void BinTree::makeEmpty()
{
makeEmptyHelper(root);
}

void BinTree::makeEmptyHelper(Node*& current)
{
if (current != NULL)
{
if (current->data != NULL)
{
delete current->data;
current->data = NULL;
}
makeEmptyHelper(current->left);
makeEmptyHelper(current->right);
delete current;
current = NULL;
}
delete current;
}

bool BinTree::insert(NodeData* insertion)
{
return insertHelper(root, insertion);
}

bool BinTree::insertHelper(Node*& current, NodeData* insertion)
{
if (current == NULL)
{
current = new Node;
current->data = insertion;
current->left = NULL;
current->right = NULL;
//delete insertion;
}
else if (*insertion < *current->data)
{
insertHelper(current->left, insertion);
}
else if (*insertion > *current->data)
{
insertHelper(current->right, insertion);
}
else
{
return false;
}
return true;
}

void buildTree(BinTree&, ifstream&);

int main()
{
ifstream infile("data2.txt");
if (!infile)
{
cout << "File could not be opened." << endl;
return 1;
}

BinTree T;
cout << "initial data:" << endl << " ";
buildTree(T, infile);
cout << endl;

NodeData* p; // pointer of retrieved object
bool found; // whether or not object was found in tree

return 0;
}

void buildTree(BinTree& T, ifstream& infile)
{
string s;

for (;;)
{
infile >> s;
cout << s << ' ';
if (s == "$$") break;
if (infile.eof()) break;

NodeData* ptr = new NodeData(s);

bool success = T.insert(ptr);
if (!success)
delete ptr;
}
}


thanks

valgrind --leak-check=full --show-leak-kinds=all ./a.out
==2201== Memcheck, a memory error detector
==2201== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==2201== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==2201== Command: ./a.out
==2201==
initial data:
iii not tttt eee r not and jj r eee pp r sssss eee not tttt ooo ff m m y z $$
==2201==
==2201== HEAP SUMMARY:
==2201== in use at exit: 72,960 bytes in 9 blocks
==2201== total heap usage: 40 allocs, 31 frees, 83,512 bytes allocated
==2201==
==2201== 256 bytes in 8 blocks are definitely lost in loss record 1 of 2
==2201== at 0x4C2A0FC: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2201== by 0x401820: buildTree(BinTree&, std::basic_ifstream<char, std::char_traits<char> >&) (new.cpp:187)
==2201== by 0x401718: main (new.cpp:168)
==2201==
==2201== 72,704 bytes in 1 blocks are still reachable in loss record 2 of 2
==2201== at 0x4C29BBE: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2201== by 0x4EC0A1F: pool (eh_alloc.cc:123)
==2201== by 0x4EC0A1F: __static_initialization_and_destruction_0 (eh_alloc.cc:250)
==2201== by 0x4EC0A1F: _GLOBAL__sub_I_eh_alloc.cc (eh_alloc.cc:326)
==2201== by 0x400F4F9: call_init.part.0 (in /usr/lib/ld-2.24.so)
==2201== by 0x400F60A: _dl_init (in /usr/lib/ld-2.24.so)
==2201== by 0x4000DA9: ??? (in /usr/lib/ld-2.24.so)
==2201==
==2201== LEAK SUMMARY:
==2201== definitely lost: 256 bytes in 8 blocks
==2201== indirectly lost: 0 bytes in 0 blocks
==2201== possibly lost: 0 bytes in 0 blocks
==2201== still reachable: 72,704 bytes in 1 blocks
==2201== suppressed: 0 bytes in 0 blocks
==2201==
==2201== For counts of detected and suppressed errors, rerun with: -v
==2201== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Answer Source

Your insertHelper code is broken because when it calls insertHelper recursively, it ignores the return value and returns true even if the object wasn't inserted, leading to a leak.

Instead of this:

    else if (*insertion < *current->data)
    {
            insertHelper(current->left, insertion);
    }
    else if (*insertion > *current->data)
    {
            insertHelper(current->right, insertion);
    }

You need this:

    else if (*insertion < *current->data)
    {
            return insertHelper(current->left, insertion);
    }
    else if (*insertion > *current->data)
    {
            return insertHelper(current->right, insertion);
    }

Why are you using naked pointers anyway?