Please help me resolve this program keeps crashing

[SMA_01]

I'm writing a program in C++ that reads integer values from a file. I am trying to run it but it either keeps crashing or running uncontrollably. Here is the function where the problem is:

void createList(intNode*& intList)
{
    intNode* lastInt; //points to last integer in file
    lastInt = NULL;
    int fileInt; //int read from input file
    ifstream intInputFile;

    intInputFile.open("intInput.txt");
    if (intInputFile.is_open())
    {
        cout << "intInput.txt open successful" << endl;
    }
    else
    {
        cout << "intInput.txt open unsuccessful" << endl;
    }
    intInputFile >> fileInt;
    cout << "check" <<endl;
    while(!intInputFile.eof())
    {
        intNode* anotherInt;
        anotherInt = new intNode;
    [COLOR="Red"] [B]  if(intList==NULL)
       {
           intList = anotherInt;
           lastInt = anotherInt;
           lastInt->nextNode = new intNode;
       }
       else
       {
          lastInt->nextNode = new intNode;
          lastInt = lastInt->nextNode;
          lastInt->nextNode = NULL;
       }
       lastInt->intValue = fileInt;
       intInputFile >> fileInt;
       cout << "good" <<endl;[/B][/COLOR]
    }
    cout <<"whats the sitch"<<endl;
    intInputFile.close();
    cout << "List created from input file" << endl;
}

The area in bold red is the problem, but I can't figure out what exactly the issue is. Any help please?

Thanks.

 

[aralbrec]

There are several problems but the one that is causing the crash is this:

1. Enter the function with intList != NULL
2. lastInt = NULL
3. In the else part of your section in red, lastInt->nextNode = new intNode

will cause a segmentation fault.


You've got lots of problems with memory leaks -- you may want to rethink how you form your linked list.

In each iteration of your while loop you create a new intNode and assign it to anotherInt. It doesn't actually get used anywhere (except possibly one time in the first part of the if block in red). Each time through the loop, anotherInt is allocated another intNode and a pointer to the previous allocation is lost forever.

if intList==NULL, the first part of the red if block is executed once. Inside there you create a list of one node that will be assigned with the read value after the if block is executed. But you also create another intNode and chain it to the end(?) without advancing the lastInt pointer. This extra node will be lost forever in the next loop iteration when the other part of the if block is executed.

Anyway have a look and see if those problems can be fixed :)

 

[SMA_01]

Thank you, what do you mean by segmentation fault? Sorry, I'm not much of a programmer so I couldn't really understand everything you said, can you please clarify? I've tried drawing out the problem, and this it's basically same code, just a bit different:

while(!intInputFile.eof())
    {
        intNode* anotherInt;
        anotherInt = new intNode;
       if(intList==NULL)
       {
           intList = anotherInt;
           lastInt = anotherInt;
           lastInt->nextNode = NULL;
           lastInt->nextNode = new intNode;
       }
       else
       {
          lastInt = lastInt->nextNode;
          lastInt->nextNode = NULL;
          lastInt->nextNode = new intNode;
       }
       lastInt->intValue = fileInt;
       intInputFile >> fileInt;
       cout << "good" <<endl;
    }

I added the cout << "good" part to see if it will reach there, and then my program just kept saying "good" uncontrollably. It seemed like this code will work fine, because intList is initially set to NULL, do you think it's an issue with my file?

 

[aralbrec]

I added the cout << "good" part to see if it will reach there, and then my program just kept saying "good" uncontrollably.

This is because the cout is inside the while loop. Each time an integer is read, that line is executed.

It seemed like this code will work fine, because intList is initially set to NULL, do you think it's an issue with my file?

Well it's not initially set to NULL :) You're depending on the caller to set it to NULL for you when you should be doing it inside the function. Someone who is unfamiliar with the code might think your function is able to append integers to an existing list because the int*& in the parameter is suggesting that. In other words, it is a potential bug for someone else down the line.

A better way is to return the list as return value and not accept a parameter. Eg, like this:

intNode *createList(void)
{
...
}

Then it's very clear to anyone else reading your code that a new list is being returned. Maintenance is a big issue in the real world and a lot of software engineering is about being defensive.

Thank you, what do you mean by segmentation fault?

A segmentation fault is accessing memory that does not belong to the process (program) or accessing memory in a way that is disallowed (eg trying to write to a read-only segment). In modern computers, this is normally detected by the hardware and will cause an interrupt that the operating system will usually respond to be creating a core dump and terminating the program.

In the scenario I gave you, you had lastInt=NULL, which is usually 0. The following assignment lastInt->nextNode=new intNode would have tried to store the pointer to the new intNode at an address close to 0, which is well outside the address range allowed for variables (on purpose and this is set up by the compiler/linker/operating system to detect such errors) and would trigger a segmentation fault.

while(!intInputFile.eof())
    {
        intNode* anotherInt;
        anotherInt = new intNode;
[COLOR="Red"]// ^^ Here you are creating an intNode every time you read an integer but
// it is never used except for the first time through this loop when the
// first block of the following 'if' statement is executed.
//
// Every other time, you create a new intNode and overwrite the address
// of the intNode you created on the previous loop iteration.  This means
// you've lost a reference to the last intNode forever and there is no way
// to reclaim the memory you allocated for it.  This is called a memory leak --
// your program is slowly using up memory with no way to return it.[/COLOR]

       if(intList==NULL)
       {
           intList = anotherInt;
           lastInt = anotherInt;
           lastInt->nextNode = NULL;
           lastInt->nextNode = new intNode;
       }
       else
       {
          lastInt = lastInt->nextNode;
          lastInt->nextNode = NULL;
          lastInt->nextNode = new intNode;
       }
       lastInt->intValue = fileInt;
       intInputFile >> fileInt;
       cout << "good" <<endl;
    }

In the 'if' above, you are always creating one extra intNode for the next integer, which means when the function returns you will always have one extra void intNode at the end of the list. You should only create intNodes as you need them, when you have an integer to store.

See if it still crashes after these things are fixed.

 

[SMA_01]

Thank you!