stset stset - 1 month ago 11
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

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?

Comments