Thread: inexplicable access violation

  1. #1
    Registered User
    Join Date
    Aug 2009
    Posts
    39

    inexplicable access violation

    Although, I have been looking for hours in my program for memory management mistakes, without finding any, I keep getting the "access violation" error. According to the debugging, it seems to occur at an it++ line. Let me post you the whole code. If you gave me your lights, it would be highly appreciated.

    Code:
    //metro.h
    #define totalWaggons 10
    #define maxCapacity 40
    using namespace std;
    #include <list>
    
    class train{
          public:
                 train(int);
                 ~train(void);
                 void printStatistics(void);
                 void operate();
                 void income(int);
          private:
                  int money;
                  int stations;
    };
    
    class passenger{
          public:
                 passenger(int, int);
                 int have_ticket();
                 int may_discount();
          private:
                  int ticket;
                  int discount;
    };
    
    class waggon{
          public:
                 waggon();
                 ~waggon();
                 void embark(int);
                 void disembark(int);
                 void inStation();
                 void betweenStations();
                 void printStatistics(void);
          private:
                  int offenders;
                  int escapers;
                  int passengers;
                  list<class passenger *> passlist;
                  
    };
    Code:
    //metro.cpp
    #include "metro.h"
    #include <iostream>
    #include <cstdlib>
    #include <ctime>
    train *T;
    
    train::train(int s){
         cout << "A metro train with " << totalWaggons << " waggons, was created" << endl;
         stations = s;
         money=0;
         }
         
    train::~train(){
         cout << "A metro train was destroyed" << endl;
         }
    
    void train::operate(void){
        waggon* w;
        w = new waggon[totalWaggons];
        for(int i=0; i<stations; i++){
           for(int j=0; j<totalWaggons; j++){
             w[j].inStation();
             if(rand()%2)
               w[j].betweenStations();
             w[j].printStatistics();
           }
        printStatistics();
        }
        delete[] w;
    }  
    
    void train::income(int amount){
         money += amount;
    }
         
    waggon::waggon(void){
         cout << "A waggon with capacity for " << maxCapacity << " passengers, was created" << endl;
         passengers=offenders=escapers=0;
    }
         
    waggon::~waggon(void){
         cout << "A waggon was destroyed" << endl;
    }
    
    void waggon::printStatistics(void){
         cout << offenders << " passengers were found to offend" << endl;
         cout << escapers << " without ticket were not caught" << endl;
    }
    
    void train::printStatistics(void){
         cout << money << " euros were collected" << endl;
         }
    
    void waggon::disembark(int n){
         list<passenger *>::iterator it;
         for(it=passlist.begin(); n>1; n--)
            it++;
         passlist.erase(it);
    }
    
    void waggon::embark(int n){
        list<passenger *>::iterator it;
        passenger *p;
        p = new passenger(rand()%2, rand()%2);
        if(!p->have_ticket())
              escapers++;
        else{
              if(p->may_discount()) T->income(1);
              else T->income(2);}
        for(it=passlist.begin(); n>1; n--)
            it++;
        passlist.insert(it, p);
    }
    
    void waggon::inStation(){
         int in, out, i, rnd;
         if(passengers){
            out = rand() % passengers + 1;
            for(i=0; i<out; i++){
              rnd = rand() % passengers +1;
              disembark(rnd);
              passengers--;
            }
         }
         in = rand() % (maxCapacity-passengers) + 1;
         if(passengers==0){
           embark(1);
           passengers++; in--;
         }
         for(i=0; i<in; i++){
            rnd = rand() % passengers +1;
            embark(rnd);
            passengers++;
         }
    }
    
    void waggon::betweenStations(){
         list< passenger *>::iterator it;
         it=passlist.begin();
         while(it!=passlist.end()){
            if((*it)->have_ticket() == 0){
               offenders++;
               escapers--;
               if((*it)->may_discount())
                  T->income(30);
               else
                  T->income(60);
               it = passlist.erase(it);
            }
            else
              it++;
         } 
    }
    
    passenger::passenger(int tick, int disc){
      ticket = tick;
      disc = discount;
    }
    
    int passenger::have_ticket(){
        return ticket;
    }
    
    int passenger::may_discount(){
        return discount;
    }
            
    int main(int argc, char *argv[]){
        int stations;
        if(argc>1){
           srand(time(NULL));
           stations = atoi(argv[1]);
           if(stations>0){
              T = new train(stations);
              T->operate();
              }
        }
        else
        cout << "Please define number of stations" << endl;
        system("pause");
    }

  2. #2
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    This doesn't look particularly safe:
    Code:
        for(it=passlist.begin(); n>1; n--)
            it++;
    Perhaps add an assert that n <= passlist.size() and then use std::advance instead of that loop.

    I consider a function that treats a list as though it were an array (i.e. disembark) to be an abomination.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  3. #3
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,817
    Code:
    void waggon::embark(int n){
        list<passenger *>::iterator it;
        passenger *p;
        p = new passenger(rand()%2, rand()%2);
        if(!p->have_ticket())
              escapers++;
        else{
              if(p->may_discount()) T->income(1);
              else T->income(2);}
        for(it=passlist.begin(); n>1; n--)
            it++;
        passlist.insert(it, p);
    }
    Your test condition here does not do anything to prevent it from going beyond the end of the list. There is also nothing preventing you passing an invalid iterator to the insert function, you want to make sure it points to something valid before you try to insert into the list at that position.


    Code:
    //metro.h
    #define totalWaggons 10
    #define maxCapacity 40
    using namespace std;
    #include <list>
    
    ...
    
    class waggon{
        ...
        list<class passenger *> passlist;
              
    };
    Don't put that using namespace statement in a header. If you need to do so, do it at the source level after all headers have been included. Explicitly qualify any objects that need it:
    Code:
    //metro.h
    #define totalWaggons 10
    #define maxCapacity 40
    #include <list>
    
    ...
    
    class waggon{
        ...
        std::list<class passenger *> passlist;
              
    };
    Your metro.cpp source file will need the using namespace std bit in that file.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  4. #4
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Here are some suggestions, roughly in the order that I observed from your code:
    • Avoid using #define to define constants. Use the const keyword; in this case they might be static const member variables.
    • Do not place using directives in header files at file scope, or before a #include. That is, move using namespace std; to metro.cpp and fully qualify the names in metro.h instead (use std::list instead of list).
    • Be const correct: if the member function does not change the observable state of the object, declare it const.
    • Include the parameter names in the function prototype.
    • passenger's have_ticket and may_discount member functions look like they should return bool rather than int.
    • waggon probably should be named wagon
    • The passlist member variable probably should be a std::list<passenger> instead. If you insist on keeping it as a std::list<passenger*>, and then also using dynamic memory allocation, then you should implement the copy constructor and copy assignment operator of the waggon (wagon) class, as well as properly implement the destructor. Failure to do so can result in the "access violation" error that you are facing now, should a waggon (wagon) object be copied.
    • Avoid the use of global variables: the T global variable really should be a local variable in the global main function. It does not look like it needs to be a pointer, so don't make it a pointer.
    • Use constructor initialiser lists, e.g.,
      Code:
      train::train(int s) : money(0), stations(s) {
          cout << "A metro train with " << totalWaggons << " waggons, was created" << endl;
      }
    • Indent your code a little better, making sure that the indent levels are consistent.
    • Use container classes. You already used std::list, so why not use std::vector<waggon> instead of directly using dynamic memory allocation?
    • Prefer pre-increment to post-increment, unless you are actually making use of the effects of post-increment.
    • waggon::disembark appears to require random access. Perhaps a deque or vector will be more appropriate than a doubly linked list here.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  5. #5
    Registered User
    Join Date
    Aug 2009
    Posts
    39
    Quote Originally Posted by iMalc View Post
    This doesn't look particularly safe:
    Code:
        for(it=passlist.begin(); n>1; n--)
            it++;
    Perhaps add an assert that n <= passlist.size() and then use std::advance instead of that loop.

    I consider a function that treats a list as though it were an array (i.e. disembark) to be an abomination.
    Since n ranges from 1 to number of passengers, therefore within the limits of the list, there is no need for such an assert. I used std::advance, but I still get access violation.

  6. #6
    Registered User
    Join Date
    Aug 2009
    Posts
    39
    Quote Originally Posted by laserlight View Post
    [*]The passlist member variable probably should be a std::list<passenger> instead. If you insist on keeping it as a std::list<passenger*>, and then also using dynamic memory allocation, then you should implement the copy constructor and copy assignment operator of the waggon (wagon) class, as well as properly implement the destructor. Failure to do so can result in the "access violation" error that you are facing now, should a waggon (wagon) object be copied.
    I am not sure I fully understand you. How shall I implement the copy constructor and assignment operator of the waggon class and how does this relate to the access violation errors? Could you show me by code?
    Thanks for the rest of your comments. They will be taken into consideration.

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by vril
    Since n ranges from 1 to number of passengers, therefore within the limits of the list, there is no need for such an assert.
    You should assert anyway, just in case your assumption proves to be incorrect.

    Quote Originally Posted by vril
    How shall I implement the copy constructor and assignment operator of the waggon class and how does this relate to the access violation errors?
    I would prefer just making the member variable a std::list<passenger>. The potential problem now is that if a waggon object is copied, its copy has a std::list of passenger pointers that point to the same passenger objects as the original waggon object. This might be fine, or it might not.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #8
    Registered User
    Join Date
    Aug 2009
    Posts
    39
    Quote Originally Posted by laserlight View Post
    I would prefer just making the member variable a std::list<passenger>. The potential problem now is that if a waggon object is copied, its copy has a std::list of passenger pointers that point to the same passenger objects as the original waggon object. This might be fine, or it might not.
    I prefer to keep it as std::list<passenger *>. Is there any idea what causes the access violation and how it can be avoided?

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by vril
    I prefer to keep it as std::list<passenger *>.
    Why? I can tell you that doing it as per my preference is simply easier to be correct.

    Quote Originally Posted by vril
    Is there any idea what causes the access violation and how it can be avoided?
    You have presumably updated your code based on the various suggestions that you have received. What is your current code? Furthermore, use a debugger, set breakpoints and step through your program. Where is the access violation triggered? This can help you to determine the start of the portion of your code which you can ignore when searching for this mistake (but it does not always tell you exactly where the problem itself lies: the problem could have surfaced due to something else).
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    Quote Originally Posted by vril View Post
    Since n ranges from 1 to number of passengers, therefore within the limits of the list, there is no need for such an assert.
    If everyone thought like that then asserts would never get used!

    n is a parameter to the function. It could get called from anywhere else later on and the value might be out of range. The assert acts as a precondition check. Not only that, but if you've ever worked in a team you'd realise that the assert has the effect of documenting the assumption your code is making so that others may see what you're assuming.
    This is actually the perfect candidate for an assert.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  11. #11
    Registered User
    Join Date
    Aug 2009
    Posts
    39
    Quote Originally Posted by iMalc View Post
    If everyone thought like that then asserts would never get used!

    n is a parameter to the function. It could get called from anywhere else later on and the value might be out of range. The assert acts as a precondition check. Not only that, but if you've ever worked in a team you'd realise that the assert has the effect of documenting the assumption your code is making so that others may see what you're assuming.
    This is actually the perfect candidate for an assert.
    You were right. For some reason assertion fails...
    edit: I found it! When a passenger witout ticket disembarked, I forgot to decrement total passengers. I fixed it and the program doesn't crash anymore. However, I am now facing another strange complication. The while-block inside betweenStations becomes an endless loop, as if the iterator never equals passlist.end.
    Last edited by vril; 11-30-2010 at 02:19 PM.

  12. #12
    Registered User
    Join Date
    Aug 2009
    Posts
    39
    I found it once again. It was because of advance(it, n), which result in deleting the end node of the list. I corrected it with advance(it, n-1).

  13. #13
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    Awesome, glad to hear it, and I'm sure it serves very well to demonstrate how asserts are wickedly great tool!

    Use them whenever your program makes an assumption that if false, would result in your program misbehaving. You leave them in the final program by the way, they don't affect release code, only debug.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. An Access Violation Segmentation Fault. Need Help ASAP
    By darknessfalls in forum C Programming
    Replies: 2
    Last Post: 08-22-2010, 05:56 AM
  2. Access Violation when writing to String
    By emef in forum C Programming
    Replies: 4
    Last Post: 11-08-2009, 10:42 PM
  3. Access violation... can't figure it out...
    By Raigne in forum C++ Programming
    Replies: 7
    Last Post: 10-11-2007, 10:52 AM
  4. FtpFileFind access violation with MS VC++ 6.0
    By earth_angel in forum C++ Programming
    Replies: 3
    Last Post: 09-22-2005, 07:02 PM
  5. 0xC0000005: Access Violation
    By Strider in forum Windows Programming
    Replies: 3
    Last Post: 11-07-2001, 02:46 PM