Thread: Memory leak - need help finding

  1. #1
    Chad Johnson
    Join Date
    May 2004
    Posts
    154

    Question Memory leak - need help finding

    Alright guys, I have a class called Bible. I have a memory leak when I create a new Bible object (being a pointer), then delete the object, the recreate it using a different file bible source file for the constructor parameter.

    It's about 4.5 - 5MB, and I don't know what I'm doing wrong. I also have Book, Chapter, and Verse classes which go along with the Bible class. It's all a tree structure.

    I'm thinking maybe it's cause a lot of my class members are not pointers, just normal stack variables, so they're not being deleted properly when the object is destroyed...?

    Here's the source file. Any insight would be wonderful.

    *EDIT*
    nevermind the source file, I'll just post the code here.
    Code:
    /******************************************************************************
     *
     * This file is part of BibleReader.
     *
     * BibleReader is free software; you can redistribute it and/or modify
     * it under the terms of the GNU General Public License as published by
     * the Free Software Foundation; either version 2 of the License, or
     * (at your option) any later version.
     *
     * This program is distributed in the hope that it will be useful,
     * but WITHOUT ANY WARRANTY; without even the implied warranty of
     * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
     * GNU General Public License for more details.
     *
     * You should have received a copy of the GNU General Public License
     * along with this program; if not, write to the Free Software
     * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
     *
     */
    
    #ifndef BIBLESTRUCTURES_H
    #define BIBLESTRUCTURES_H
    
    #include <iostream>
    #include <sstream>
    #include <fstream>
    #include <vector>
    #include <queue>
    
    using namespace std;
    
    // class prototypes
    class SearchResult;
    class Verse;
    class Chapter;
    class Book;
    class Bible;
    
    // constants
    const int LENGTH_BYTES = 2;
    const int LOCATION_BYTES = 4;
    const int COUNT_BYTES = 2;
    
    class SearchResult
    {
     private:
        int book;
        int chapter;
        int verse;
    
     public:
        SearchResult(int b, int c, int v)
        {
            book = b;
            chapter = c;
            verse = v;
        }
    
        ~SearchResult()
        {
        }
    
        const int GetBook() const
        {
            return book;
        }
    
        const int GetChapter() const
        {
            return chapter;
        }
    
        const int GetVerse() const
        {
            return verse;
        }
    };
    
    
    class Verse
    {
     private:
        int pos;
        string text;
    
     public:
        Verse(const int p)
        {
            pos = p;
        }
    
        Verse(const int p, string t)
        {
            pos = p;
            text = t;
        }
    
        ~Verse()
        {
        }
    
        const int GetVersePos() const
        {
            return pos;
        }
    
        string GetText()
        {
            return text;
        }
    };
    
    
    class Chapter
    {
     private:
        int pos;
        int nVerses;
        vector<Verse*> verse_list;
    
     public:
        Chapter(const string *buffer, long p);
    
        ~Chapter()
        {
        }
    
        const int GetVerseCount() const
        {
            return nVerses;
        }
    
        const int GetChapterPos() const
        {
            return pos;
        }
    
        Verse *GetVerse(int v) const
        {
            if (v > nVerses || v < 0)
                return NULL;
    
            return verse_list[v];
        }
    };
    
    
    class Book
    {
     private:
        string book_name;
        int pos;
        int nChapters;
        vector<Chapter*> chapter_list;
    
     public:
        Book(const string *buffer, long p);
    
        ~Book()
        {
        }
    
        const string GetBookName() const
        {
            return book_name;
        }
    
        const int GetChapterCount() const
        {
            return nChapters;
        }
    
        const int GetBookPos() const
        {
            return pos;
        }
    
        Chapter *GetChapter(int c) const
        {
            if (c > nChapters || c < 0)
                return NULL;
    
            return chapter_list[c];
        }
    };
    
    
    class Bible
    {
     private:
        string title;
        string copyright;
        vector<Book*> book_list;
        int nBooks;
        string *buffer;
    
     public:
        Bible(const char *fname);
    
        ~Bible()
        {
        }
    
        bool InputFileToMemory(const char *fname);
        queue<SearchResult*> *Find(string str);
    
        const string GetTitle() const
        {
            return title;
        }
    
        const string GetCopyright() const
        {
            return copyright;
        }
    
        Book *GetBook(int b) const
        {
            if (b > nBooks || b < 0)
                return NULL;
    
            return book_list[b];
        }
    
        const int GetBookCount() const
        {
            return nBooks;
        }
    
        const string GetVerseText(int b, int c, int v) const
        {
            if (b > nBooks || b < 0)
                return string("");
    
            if (c > GetBook(b)->GetChapterCount() || c < 0)
                return string("");
    
            if (v > GetBook(b)->GetChapter(c)->GetVerseCount() || v < 0)
                return string("");
    
            return GetBook(b)->GetChapter(c)->GetVerse(v)->GetText();
        }
    };
    
    Chapter::Chapter(const string *buffer, long p)
    {
        istringstream tmp_buffer;
        long verse_pos = 0;
        int len = 0;
        string verse_text = "";
        int i = 0;
    
        // store chapter location incase needed for future reference
        pos = p;
        nVerses = 0;
    
        // number of verses in chapter
        tmp_buffer.str(buffer->substr(p, COUNT_BYTES));
        tmp_buffer.read((char*)&nVerses, COUNT_BYTES);
        p += COUNT_BYTES;
    
        // set up verses
        for (i=0; i<nVerses; i++)
        {
            // verse location
            tmp_buffer.str(buffer->substr(p, LOCATION_BYTES));
            tmp_buffer.read((char*)&verse_pos, LOCATION_BYTES);
            p += LOCATION_BYTES;
    
            // check that verse has not been left out of Bible. If it has, insert an empty string for the verse text
            if (verse_pos > 0)
            {
                // verse text length
                tmp_buffer.str(buffer->substr(verse_pos, LENGTH_BYTES));
                 tmp_buffer.read((char*)&len, LENGTH_BYTES);
    
                // verse text
                verse_text = buffer->substr(verse_pos+LENGTH_BYTES, len);
    
                // insert new book into book list
                verse_list.push_back(new Verse(verse_pos, verse_text));
            }
            else
                verse_list.push_back(new Verse(verse_pos, string("")));
        }
    }
    
    Book::Book(const string *buffer, long p)
    {
        istringstream tmp_buffer;
        long chapter_pos = 0;
        int len = 0;
        int i = 0;
    
        // store book location incase needed for future reference
        pos = p;
        nChapters = 0;
        book_name = "";
    
        // book name length
        tmp_buffer.str(buffer->substr(p, LENGTH_BYTES));
        tmp_buffer.read((char*)&len, LENGTH_BYTES);
        p += LENGTH_BYTES;
    
        // book name
        book_name = buffer->substr(p, len);
        p += len;
    
        // number of chapters in book
        tmp_buffer.str(buffer->substr(p, COUNT_BYTES));
        tmp_buffer.read((char*)&nChapters, COUNT_BYTES);
        p += COUNT_BYTES;
    
        // set up chapters
        for (i=0; i<nChapters; i++)
        {
            // chapter location
            tmp_buffer.str(buffer->substr(p, LOCATION_BYTES));
            tmp_buffer.read((char*)&chapter_pos, LOCATION_BYTES);
            p += LOCATION_BYTES;
    
            // insert new book into book list
            chapter_list.push_back(new Chapter(buffer, chapter_pos));
        }
    }
    
    Bible::Bible(const char *fname)
    {
        istringstream tmp_buffer;
        long buffer_offset = 0;
        long book_pos = 0;
        int len = 0;
        int i = 0;
    
        // TODO: better error checking here
        if (!InputFileToMemory(fname))
            return;
    
        // title length
        tmp_buffer.str(buffer->substr(buffer_offset, LENGTH_BYTES));
        tmp_buffer.read((char*)&len, LENGTH_BYTES);
        buffer_offset += LENGTH_BYTES;
    
        // title
        title = buffer->substr(buffer_offset, len);
        buffer_offset += len;
    
        // copyright length
        tmp_buffer.str(buffer->substr(buffer_offset, LENGTH_BYTES));
        tmp_buffer.read((char*)&len, LENGTH_BYTES);
        buffer_offset += LENGTH_BYTES;
    
        // copyright
        copyright = buffer->substr(buffer_offset, len);
        buffer_offset += len;
    
        // number of books
        tmp_buffer.str(buffer->substr(buffer_offset, COUNT_BYTES));
        tmp_buffer.read((char*)&nBooks, COUNT_BYTES);
        buffer_offset += COUNT_BYTES;
    
        // set up books
        // get the book locations, then create a new Book object and let do the rest
        for (i=0; i<nBooks; i++)
        {
            // book location
            tmp_buffer.str(buffer->substr(buffer_offset, LOCATION_BYTES));
            tmp_buffer.read((char*)&book_pos, LOCATION_BYTES);
            buffer_offset += LOCATION_BYTES;
    
            // insert new book into book list
            book_list.push_back(new Book(buffer, book_pos));
        }
    
        delete buffer;
    }
    
    // cache bible file in memory for faster access
    // return true on success, false on error
    bool Bible::InputFileToMemory(const char *fname)
    {
        fstream in_file;
        char *tmp_buffer;
        long fsize;
    
        in_file.open(fname, ios::in|ios::binary|ios::ate);
    
        if (in_file == NULL)
            return false;
    
        // get size of file
        fsize = in_file.tellg();
        in_file.seekg(0, ios::beg);
    
        tmp_buffer = new char[fsize];
    
        // read file into buffer
        in_file.read(tmp_buffer, fsize);
        in_file.close();
    
        buffer = new string(tmp_buffer, fsize);
    
        delete tmp_buffer;
    
        return true;
    }
    
    queue<SearchResult*> *Bible::Find(string str)
    {
        queue<SearchResult*> *results;
        int i, j, k;
    
        results = new queue<SearchResult*>;
    
        for (i=0; i<GetBookCount(); i++)
        {
            for (j=0; j<GetBook(i)->GetChapterCount(); j++)
            {
                for (k=0; k<GetBook(i)->GetChapter(j)->GetVerseCount(); k++)
                {
                    // check that verse has not been left out of Bible (empty string). If it has, go to the next verse
                       if (GetVerseText(i, j, k).length() == 0)
                        continue;
    
                    if (GetVerseText(i, j, k).find(str) != -1)
                        results->push(new SearchResult(i, j, k));
                }
            }
        }
    
        return results;
    }
    
    #endif
    Last edited by ChadJohnson; 04-05-2005 at 01:45 PM.

  2. #2
    Anti-Poster
    Join Date
    Feb 2002
    Posts
    1,401
    Hmm...yup, a lot of new's that don't get deleted.
    Code:
                verse_list.push_back(new Verse(verse_pos, verse_text));
    This allocates your memory. In your class destructor, you need to go back through your verse_list and delete it. The vector class will not do this for you. Similarly with the chapter_list and book_list.
    Code:
    queue<SearchResult*> *Bible::Find(string str)
    {
        queue<SearchResult*> *results;
        int i, j, k;
    
        results = new queue<SearchResult*>;
        //snip
        
        return results;
    }
    I understand what you're doing here. It's bad. That leaves it up to the outside world to delete the results. Instead, restructure your function declaration to use an output parameter:
    Code:
    void Bible::Find(string str, queue<SearchResult*>& results)
    If I did your homework for you, then you might pass your class without learning how to write a program like this. Then you might graduate and get your degree without learning how to write a program like this. You might become a professional programmer without knowing how to write a program like this. Someday you might work on a project with me without knowing how to write a program like this. Then I would have to do you serious bodily harm. - Jack Klein

  3. #3
    Chad Johnson
    Join Date
    May 2004
    Posts
    154
    *EDIT* deleting the vector components when the objects were deleted definitely worked. Thanks.

    So with an output parameter like that, being by reference, will the outside not have to delete it? Who deletes it?

    Also, SHOULD I make every class member a pointer?
    Last edited by ChadJohnson; 04-05-2005 at 09:03 PM.

  4. #4
    Anti-Poster
    Join Date
    Feb 2002
    Posts
    1,401
    Quote Originally Posted by ChadJohnson
    So with an output parameter like that, being by reference, will the outside not have to delete it? Who deletes it?
    Well, that's a good point. I did miss that the queue was yet again storing pointers. The SearchResult class is relatively small. I suggest trying something like this:
    Code:
    void Bible::Find(string str, queue<SearchResult>& results)
    {
        //snip
                        if (GetVerseText(i, j, k).find(str) != -1)
                            results->push(SearchResult(i, j, k));
    }
    Then there's no messy allocating or deallocating of memory necessary. To call from the outside, all you need is something like this:
    Code:
    int main()
    {
       Bible theBible;
       string searchQuery;
       queue<SearchResult> output;
       //do stuff
       theBible.Find(searchQuery,output);
       //do other stuff
       return 0;
    }
    Quote Originally Posted by ChadJohnson
    Also, SHOULD I make every class member a pointer?
    I wouldn't. There's no real need.
    If I did your homework for you, then you might pass your class without learning how to write a program like this. Then you might graduate and get your degree without learning how to write a program like this. You might become a professional programmer without knowing how to write a program like this. Someday you might work on a project with me without knowing how to write a program like this. Then I would have to do you serious bodily harm. - Jack Klein

  5. #5
    Chad Johnson
    Join Date
    May 2004
    Posts
    154
    The SearchResult class is used in conjunction with a GUI control - a list box. One object is stored for each item in the list, so there can be up to 30000 SearchResult objects in a list.

    So do you think I should do what you said in your last post and still not worry abut making the objects in the queue pointers, just stack variables?

  6. #6
    Anti-Poster
    Join Date
    Feb 2002
    Posts
    1,401
    Look at it this way. No pointers = 30000 SearchResult objects in the queue. Pointers = 30000 SearchResult pointers in the queue + 30000 SearchResult objects floating around elsewhere.
    If I did your homework for you, then you might pass your class without learning how to write a program like this. Then you might graduate and get your degree without learning how to write a program like this. You might become a professional programmer without knowing how to write a program like this. Someday you might work on a project with me without knowing how to write a program like this. Then I would have to do you serious bodily harm. - Jack Klein

  7. #7
    Carnivore ('-'v) Hunter2's Avatar
    Join Date
    May 2002
    Posts
    2,879
    I'd just like to point out here, they won't be stack variables no matter what you do. I'm pretty sure queue uses dynamic memory internally, so it will all go on the heap anyway.

    The main concern in deciding between storing pointers and storing objects is usually:
    a) Does the object have a suitable copy constructor/assignment operator? i.e. a copy of the original object will function in exactly the same way, with no shared resources. If not, then you must use pointers.
    b) Is the object very large? If so, you may lose efficiency in lengthy copy operations.
    b2) Is the code going to be run many times per second? If so, depending on how often objects are added/deleted, you may or may not notice slowdowns.

    In your case, the object is composed of 3 ints; (a) the default copy constructor/assignment operator will suffice, since everything is by-value and contained within the structure. (b) 3 ints is pretty much insignificant unless you're running it several billion (trillion?) times per second for an extended period of time. Looks to me like it'll be run 30,000 times max. Thus (b2) is also insignificant.

    I conclusion, I would recommend storing objects in the queue, since: (a) It makes life much easier, and (b) It makes life safer, and (c) Negative side-effects are negligible.

    Just Google It. √

    (\ /)
    ( . .)
    c(")(") This is bunny. Copy and paste bunny into your signature to help him gain world domination.

  8. #8
    Chad Johnson
    Join Date
    May 2004
    Posts
    154
    Can you explain what you mean by 'shared resources'? Also, what do you mean by an assignment operator?

  9. #9
    Carnivore ('-'v) Hunter2's Avatar
    Join Date
    May 2002
    Posts
    2,879
    >>Also, what do you mean by an assignment operator?
    Code:
    x = y; //Assignment operator of x is called, to copy y's contents into itself.
    >>Can you explain what you mean by 'shared resources'?
    Perhaps 'shared resources' isn't a perfectly accurate description. I'm not sure how much you know about the subject already, but an example of what I mean is dynamic memory. If a direct copy is made of the pointer, then both objects will access the same memory and you'll get data corruption (assuming you're not expecting it). A common example for this is dynamic memory that is freed by a destructor; if you push such an object onto a container, a copy of it is made, and when the original goes out of scope, the dynamic memory is deleted and you have a dangling pointer left in the object in the container. In this case, if the class is simple enough (or well enough designed), it should be an easy task to create a copy constructor that creates its own dynamic memory and copies the value rather than the address. However, there are cases where a copy simply will not suffice, i.e. when the resource in question cannot be copied without losing information or it would be extremely inefficient/difficult to do such a copy - such as when the resource is a large tree structure. In these cases, you require a workaround such as reference counting, so that the copied object will reference the same resource but the resource itself will not be freed until all 'copies' of the object are destroyed. Depending on the situation, this may be overkill (or you might be too lazy to do it ), and as a result we often have structures that cannot be reliably copied etc. for direct use in STL containers; this problem is solved by storing pointers to dynamically allocated objects instead.
    Just Google It. √

    (\ /)
    ( . .)
    c(")(") This is bunny. Copy and paste bunny into your signature to help him gain world domination.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Mutex and Shared Memory Segment Questions.
    By MadDog in forum Linux Programming
    Replies: 14
    Last Post: 06-20-2010, 04:04 AM
  2. Question regarding Memory Leak
    By clegs in forum C++ Programming
    Replies: 29
    Last Post: 12-07-2007, 01:57 AM
  3. Memory leak with detached pthreads - how to free?
    By rfk in forum Linux Programming
    Replies: 2
    Last Post: 08-17-2007, 06:50 AM
  4. Huge memory leak
    By durban in forum C++ Programming
    Replies: 3
    Last Post: 11-01-2005, 12:10 PM
  5. Manipulating the Windows Clipboard
    By Johno in forum Windows Programming
    Replies: 2
    Last Post: 10-01-2002, 09:37 AM