Segmentation Fault when trying to push a string to the back of a list.

Question

I am trying to write a logger class for my C++ calculator, but I'm experiencing a problem while trying to push a string into a list.

I have tried researching this issue and have found some information on this, but nothing that seems to help with my problem. I am using a rather basic C++ compiler, with little debugging utilities and I've not used C++ in quite some time (even then it was only a small amount).

My code:

#ifndef _LOGGER_H_
#define _LOGGER_H_

#include <iostream>
#include <list>
#include <string>

using std::cout;
using std::cin;
using std::endl;
using std::list;
using std::string;

class Logger
{
private:
 list<string> mEntries;

public:
 Logger() {}
 ~Logger() {}

 // Public Methods
 void WriteEntry(const string& entry)
 {
  mEntries.push_back(entry);
 }

 void DisplayEntries()
 {
  cout << endl << "**********************" << endl
            << "*   Logger Entries   *" << endl
      << "**********************" << endl
    << endl;

  for(list<string>::iterator it = mEntries.begin();
   it != mEntries.end(); it++)
  {
  // *** BELOW LINE IS MARKED WITH THE ERROR ***
   cout << *it << endl;
  }
 }
};

#endif

I am calling the WriteEntry method by simply passing in a string, like so:

mLogger->WriteEntry("Testing");

Any advice on this would be greatly appreciated.

* CODE ABOVE HAS BEEN ALTERED TO HOW IT IS NOW *

Now, the line:

cout << *it << endl;

causes the same error. I'm assuming this has something to do with how I am trying to get the string value from the iterator.

The code I am using to call it is in my main.cpp file:

#include <iostream>
#include <string>
#include <sstream>
#include "CommandParser.h"
#include "CommandManager.h"
#include "Exceptions.h"
#include "Logger.h"

using std::string;
using std::stringstream;
using std::cout;
using std::cin;
using std::endl;

#define MSG_QUIT    2384321
#define SHOW_LOGGER true

void RegisterCommands(void);
void UnregisterCommands(void);
int ApplicationLoop(void);
void CheckForLoggingOutput(void);
void ShowDebugLog(void);

// Operations
double Operation_Add(double* params);
double Operation_Subtract(double* params);
double Operation_Multiply(double* params);
double Operation_Divide(double* params);

// Variable
CommandManager  *mCommandManager;
CommandParser   *mCommandParser;
Logger          *mLogger;

int main(int argc, const char **argv)
{
    mLogger->WriteEntry("Registering commands...\0");   

    // Make sure we register all commands first
    RegisterCommands();

    mLogger->WriteEntry("Command registration complete.\0");

    // Check the input to see if we're using the program standalone,
    // or not
    if(argc == 0)
    {
        mLogger->WriteEntry("Starting application message pump...\0");

        // Full version
        int result;
        do
        {
            result = ApplicationLoop();
        } while(result != MSG_QUIT);
    }
    else
    {
        mLogger->WriteEntry("Starting standalone application...\0");

        // Standalone - single use
        // Join the args into a string
        stringstream joinedStrings(argv[0]);
        for(int i = 1; i < argc; i++)
        {
            joinedStrings << argv[i];
        }

        mLogger->WriteEntry("Parsing argument '" + joinedStrings.str() + "'...\0");

        // Parse the string
        mCommandParser->Parse(joinedStrings.str());

        // Get the command names from the parser
        list<string> commandNames = mCommandParser->GetCommandNames();

        // Check that all of the commands have been registered
        for(list<string>::iterator it = commandNames.begin();
            it != commandNames.end(); it++)
        {
            mLogger->WriteEntry("Checking command '" + *it + "' is registered...\0");

            if(!mCommandManager->IsCommandRegistered(*it))
            {
                // TODO: Throw exception
                mLogger->WriteEntry("Command '" + *it + "' has not been registered.\0");
            }
        }

        // Get each command from the parser and use it's values
        // to invoke the relevant command from the manager
        double results[commandNames.size()];
        int currentResultIndex = 0;
        for(list<string>::iterator name_iterator = commandNames.begin();
            name_iterator != commandNames.end(); name_iterator++)
        {
            string paramString = mCommandParser->GetCommandValue(*name_iterator);
            list<string> paramStringArray = StringHelper::Split(paramString, ' ');

            double params[paramStringArray.size()];
            int index = 0;
            for(list<string>::iterator param_iterator = paramStringArray.begin();
                param_iterator != paramStringArray.end(); param_iterator++)
            {
                // Parse the current string to a double value
                params[index++] = atof(param_iterator->c_str());
            }

            mLogger->WriteEntry("Invoking command '" + *name_iterator + "'...\0");

            results[currentResultIndex++] =
                mCommandManager->InvokeCommand(*name_iterator, params);
        }

        // Output all results
        for(int i = 0; i < commandNames.size(); i++)
        {
            cout << "Result[" << i << "]: " << results[i] << endl;
        }
    }

    mLogger->WriteEntry("Unregistering commands...\0");

    // Make sure we clear up our resources
    UnregisterCommands();

    mLogger->WriteEntry("Command unregistration complete.\0");

    if(SHOW_LOGGER)
    {
        CheckForLoggingOutput();
    }

    system("PAUSE");

    return 0;
}

void RegisterCommands()
{
    mCommandManager = new CommandManager();
    mCommandParser = new CommandParser();
    mLogger = new Logger();

    // Known commands
    mCommandManager->RegisterCommand("add", &Operation_Add);
    mCommandManager->RegisterCommand("sub", &Operation_Subtract);
    mCommandManager->RegisterCommand("mul", &Operation_Multiply);
    mCommandManager->RegisterCommand("div", &Operation_Divide);
}

void UnregisterCommands()
{
    // Unregister each command
    mCommandManager->UnregisterCommand("add");
    mCommandManager->UnregisterCommand("sub");
    mCommandManager->UnregisterCommand("mul");
    mCommandManager->UnregisterCommand("div");

    // Delete the logger pointer
    delete mLogger;

    // Delete the command manager pointer
    delete mCommandManager;

    // Delete the command parser pointer
    delete mCommandParser;
}

int ApplicationLoop()
{
    return MSG_QUIT;
}

void CheckForLoggingOutput()
{
    char answer = 'n';

    cout << endl << "Do you wish to view the debug log? [y/n]: ";
    cin >> answer;

    switch(answer)
    {
        case 'y':
            ShowDebugLog();
            break;
    }
}

void ShowDebugLog()
{
    mLogger->DisplayEntries();
}

// Operation Definitions
double Operation_Add(double* values)
{
    double accumulator = 0.0;

    // Iterate over all values and accumulate them
    for(int i = 0; i < (sizeof values) - 1; i++)
    {
        accumulator += values[i];
    }

    // Return the result of the calculation
    return accumulator;
}

double Operation_Subtract(double* values)
{
    double accumulator = 0.0;

    // Iterate over all values and negativel accumulate them
    for(int i = 0; i < (sizeof values) - 1; i++)
    {
        accumulator -= values[i];
    }

    // Return the result of the calculation
    return accumulator;
}

double Operation_Multiply(double* values)
{
    double accumulator = 0.0;

    for(int i = 0; i < (sizeof values) - 1; i++)
    {
        accumulator *= values[i];
    }

    // Return the value of the calculation
    return accumulator;
}

double Operation_Divide(double* values)
{
    double accumulator = 0.0;

    for(int i = 0; i < (sizeof values) - 1; i++)
    {
        accumulator /= values[i];
    }

    // Return the result of the calculation
    return accumulator;
}

---

Answer 1

You only need to do

list<string> entries;

entries.push_back();

You do not need to create a pointer to entries.

Answer 2

Nothing too obvious, though you typed

mEntries->push_back(string(entryData));

and I htink you meant entry instead of entryData. You also don't need the string conversion on that line, and your function should take entry by const reference.

However, none of these things would cause your program to segfault. What compiler are you using?

Answer 3

After your edit, the solution seem clear:

Your first line in main() is :

mLogger->WriteEntry("Registering commands...\0");  

Here mLogger is a pointer that has never been initialized. This is "undefined behaviour", meaning anything can appen, often bad things.

To fix this you can either make it a "normal" variable, not a pointer or create a Logger instance using new (either at the declaration or as the first line in main).

I suggest you to not use a pointer to be sure the logger is always there and is automatically destroyed.

By the way, it seems like you want to create every instance of objects on the heap using pointers. It's not recommanded if it's not necessary. You should use pointers ONLY if you want to explicitely state the creation (using new) and destruction (using delete) of the instance object. If you just need it in a specific scope, don't use a pointer. You might come from another language like Java or C# where all objects are referenced. If so, you should start learning C++ like a different language to avoid such kind of problem. You should learn about RAII and other C++ scpecific paradigm that you cannot learn in those languages. If you come from C you should too take it as a different language. That might help you avoid complex problems like the one you showed here. May I suggest you read some C++ pointer, references and RAII related questions on stackoverflow.

---

First, you don't need to create the std::list on the heap. You should just use it as a normal member of the class.

class Logger
{
private:
 list<string> mEntries; // no need to use a pointer

public:
 Logger() // initialization is automatic, no need to do anything
 {
 }

 ~Logger() // clearing and destruction is automatic too, no need to do anything
 {
 }
 //...
};

Next, entryData don't exist in this code so I guess you wanted to use entry. If it's not a typo then you're not providing the definition of entryData that is certainly the source of your problem.

In fact I would have written your class that way instead:

class Logger
{
private:
 list<string> mEntries;

public:
  // no need for constructor and destructor, use the default ones

 // Public Methods
 void WriteEntry(const string& entry) // use a const reference to avoid unnecessary copy (even with optimization like NRVO)
 {
  mEntries.push_back( entry ); // here the list will create a node with a string inside, so this is exactly like calling the copy constructor
 }

 void DisplayEntries()
 {
  cout << endl << "**********************" << endl
            << "*   Logger Entries   *" << endl
      << "**********************" << endl
    << endl;

  for(list<string>::iterator it = mEntries.begin();
   it != mEntries.end(); ++it) // if you want to avoid unnecessary copies, use ++it instead of it++
  {
   cout << *it << endl;
  }
 }
};

What's certain is that your segfault is from usage outside of this class.

Answer 4

Did you remember to call mLogger = new Logger at some point? Did you accidantally delete mLogger before writing to it?

Try running your program in valgrind to see whether it finds any memory errors.

Answer 5

You're missing the copy constructor. If the Logger object is copied and the original deleted, you'll be dereferencing memory that was previously deleted.

A simplified example of the problem

Logger a;
{
    Logger b;
    a=b;
}
a.WriteEntry("Testing");

Add a copy constructor.

Logger(const Logger& item)
{
    mEntries = new list<string>();
    std::copy(item.mEntries->begin(), item.mEntries->end(), std::back_inserter(*mEntries));
}

Answer 6

Is an instance of Logger being copied anywhere (either through a copy constructor or operator=)? Since you have mEntries as a pointer to a list, if you copy an instance of Logger, they will share the value of the pointer, and when one is destructed, it deletes the list. The original then has a dangling pointer. A quick check is to make the copy constructor and operator= private and not implemented:

private:
    void operator=(const Logger &); // not implemented
    Logger(const Logger &);  // not implemented

When you recompile, the compiler will flag any copies of any Logger instances.

If you need to copy instances of Logger, the fix is to follow the Rule of 3: http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29 You can do this by eliminating the need for the destructor (by not using a pointer: list<string> mEntries), or by adding the needed code to the copy constructor and operator= to make a deep copy of the list.