Pointer issue crashing program

[jojo212]

Hey guys,

One line of code keeps crashing my program and I can't figure out why.
It has something to do with a segmentation fault (or so I'm told).


First, we have the header and the constructor. These work fine:

class Train{
    public:
        Train(CTrainLine tLine, Platform start);
        void update();
        void travel();
        void board();
        
    private:
        CTrainLine trainline;
        string status,
               location;
        Platform * nextPlat, * prevPlat, * currPlat;
        int tripTime;
};


Train::Train(CTrainLine tLine, Platform start){
    location = start.getStation();
    trainline = tLine;
     
    currPlat = new Platform;
    prevPlat = new Platform;
    nextPlat = new Platform;
    
    *currPlat = start;    
    *nextPlat = tLine.getNext(start);
    prevPlat = 0;

    tripTime = 0;
    status = "boarding";
}

Then once per second, the travel() function is called for the next 3 seconds, and that looks like this:

void Train::travel(){
    currPlat->setTrain(false);
    status = "traveling";
    prevPlat = currPlat;
    currPlat = 0;
    tripTime = 0;
    location = "Between " + prevPlat->getStation() + " and " + nextPlat->getStation();
}

Then finally, after this is called 3 times, board() is called, and this is where the problem is arising:

void Train::board(){
    tripTime = 0;
    currPlat = nextPlat;

    nextPlat = new Platform;
    
    if(nextPlat && currPlat)
        (*nextPlat) = trainline.getNext(*currPlat);
    currPlat->setTrain(true);
    status = "boarding";
}

Line 8 is what's crashing my program. Usually I would expect it to mean that either nextPlat or currPlat are NULL, but neither of them are. Both of them should be pointing to a valid Platform object, because otherwise line 7 would stop it, right?

Any thoughts?

Thanks!

[MiiNiPaa]

Please give code for trainline.getNext

[jojo212]

I called trainline.getnext() at the beginning of the program and tested it several times, it's never caused me a problem. It's when I used *nextPlat that I'm getting issues.

Anyway, here's the code:

Platform CTrainLine::getNext(Platform plat){
    ListNode * curr = find(plat);
    return ((curr->next)->data);   
}

And here's' find(Platform):

struct CTrainLine::ListNode * CTrainLine::find(Platform plat){
    ListNode * curr = head;
    for(int n = 0; n < size; n++){
        curr = curr->next;
        if(curr->data == plat)
            return curr;
    }
    cout << "ERROR: ";
    plat.displayInfo();
    cout << "\n ... not found on line" << endl;
}

Both of which appear to be working fine, with some testing.

[MiiNiPaa]

what will happen, if you will pass platform contained in head ListNode?
A I can see, it will initialise curr with head, then will get next platform, and only then will test if Node contains needed data.

TL;DR: try to place routine on line 4 of find between lines 6 and 7.

[jojo212]

Tried it; still setting same issue. Error message:

An access violation (segmentation fault) occurred in your program

Not very specific at all. For what it's worth, I tried doing it this way:

  Platform currP = *currPlat;
  Platform next = trainline.getNext(currP);

And I get the same crash (on line 2, when I assign next).

The weird thing is, (*currPlat) is definitely a real platform. In fact, I called getNext() on that very same platform earlier on, no problem. Yet the program is acting as if it's void or something.

[MiiNiPaa]

Most likely find function doesn't find next platform. In that case function will terminate without returning anything. In that case random garbage value at the top of the stack will be used as return variable in line 2 of getNext function. Then you trying to dereference pointer to random memory address, which will cause segfault.

[jojo212]

I thought the issue might be with the getNext function, so in order to try and find the issue, I wrote a chunk of code into main which calls getNext() on EVERY platform in my program, calling it using pointers, the same way I do in the board() function:

Platform *a1u, *a2u, *a3u, *a1d, *a2d, *a3d;
    
    a1u = &a1UptownRed;
    a2u = &a2UptownRed;
    a3u = &a3UptownRed;
    a3d = &a3DowntownRed;
    a2d = &a2DowntownRed;
    a1d = &a1DowntownRed;
    
    
    Platform gru;
    
    gru = redLine.getNext(*a1u);
    gru = redLine.getNext(*a2u);    
    gru = redLine.getNext(*a3u);
    gru = redLine.getNext(*a3d);    
    gru = redLine.getNext(*a2d);
    gru = redLine.getNext(*a1d); 
    

This worked fine and did NOT crash, indicating to me that getNext() is not the problem (unless there's something I'm missing). This is very confusing, because I feel like that's the most likely root of the issue.

This has been a very frustrating bug.

[MiiNiPaa]

If you post full text of your program on pastebin.com, it will be easier to help find this bug

[jojo212]

Great idea, MiiniPaa.

I pasted main function and all classes into one big file. Hope that's the right way to go about it. Let me know if not.

http://pastebin.com/6XnBC29A

[jojo212]

Just to give you an update, I've narrowed down the problem some. It has to do with the find(Platform plat) function in the CTrainLine class. Before program starts, it's called on all platforms and works fine. But then during board() it's called on a2RedUptown and crashes (even though it should be returning a3RedUptown). Still not sure why this is happening.

[MiiNiPaa]

Ok, I have looked into this and found out that next pointer from second platform became invalid.

It because you had passed CTrainLine into Train constructor by value. It will make a shallow copy of the class and could lead to all sort of errors. I changed class a little and now it will take CTrainLine by reference:

class Train {
//...
Train(CTrainLine tLine, Platform start)
Train(CTrainLine &tLine, Platform start)
//...
Private:
//...
CTrainLine trainline;
CTrainLine &trainline;
//...
}

Train::Train(CTrainLine &tLine, Platform start): trainline(tLine){
    location = start.getStation();
    trainline = tLine;
//...

(change second constructor accordingly)

EDIT: better rewrite your classes to use pointers to data. i.e right now it copies platforms and it is impossible to make one platform belong to two lines simultaneously

Edit2: If you want, I can tell step by step what happened with your programm, and why that error appeared

[jojo212]

MiiNiPaa,

I haven't tried your solution yet, but based on some more experimentation I've been doing since last I posted, I think you're right. It definitely has something to do with a Platform pointer becoming invalid.


I've been working on this bug for the better part of 8 hours...

THANK YOU THANK YOU THANK YOU!!

And yes, if you wouldn't mind, please tell me step by step what's going on, because I really want to learn from this and avoid it happening again the future.

And one more time: THANK YOU!!!!!!

[cire]

Your list class does not have a copy constructor or a copy assignment operator. It needs one if you're going to be passing it around by value, and if you don't intend for it to be passed around by value, disable copy initialization and copy assignment by making those methods private (or using the "=delete" syntax if your compiler supports it) so they can't be used accidentally.

http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29
http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three

[MiiNiPaa]

You passing redLine by value and it get copied. *head pointer is copied too, but not the data it points to. Then you assign tLine to trainline in Train constructor, and pointer copy happens again. Now we have three instances of CTrainLine (redLine in main, tLine in currently running Train constructor and trainline in "pelham" Train instance) pointing to same data. When Train constructor finished, tLine goes out of scope, and its destructor called which destroys all nodes redline and trainline still pointing to. *head pointer in trainline is now invalid adn who knows what will happen, when we dereference it.

Either provide ypur own cope constructor and assigment operator, or forbid it and pass your objects by reference/pointer