Segmentation fault problem in stl map<>

Question

Hi All,

I am not very good at c++ but I have to test a reputation based system. A code fragment is given below which gives me segfault when i run it on ubuntu system. As i wrote in comments two functions "tackleFirstHandInfo()" and "updateReputation()" individually run correctly but when i call one function from the other it crashes. Any help will be greatly appreciated, thanks in advance. the code is below:

"ex.h"

#ifndef _ex_h
#define _ex_h
#include "iostream"
#include <map>
#define FADING 0.9
enum Behaviour {FORWARDING, NOTFORWARDING};

class Rating
{

private:
    double reputation;

public:

    Rating() { reputation = 5.0; }
    Rating(double rep) {reputation = rep;}

    ~Rating() {}

    double getRep() { return reputation; }

    void updateRep(Behaviour behaviour) {
        if (behaviour == FORWARDING)
            reputation = reputation + 1;    
        else 
            reputation = reputation - 1;    
    } 

};

#endif

"ex.cc"

#include <map>                                  
#include <string>
#include <iostream>   
#include "ex.h"                           
using namespace std;
typedef map<int, Rating*> ratingTable;
class RepSys {
private:
    ratingTable repTable; 
    map<int, Rating*> fHandInfo;
    Rating* rating;

public: 
    RepSys(){}
    ~RepSys(){}

    void tackleFirstHandInfo(int address, Behaviour behaviour)
    /* This Funtion and the function below individually run correctly */
    {
        map<int, Rating*>::iterator it;
        it=fHandInfo.find(address);
        if (it == fHandInfo.end()) {
            cout << "Adding New Entry for (fHandInfo) "<< address <<endl;
            rating = new Rating();   
            fHandInfo[address] = rating;
        }
        (it->second)->updateRep(behaviour);
        cout<<"First Hand Reputation of "<<address<<"\t is ="<< (it->second)->getRep()<<endl;
        updateReputation(address, behaviour); // This causes SegFault !!!!
        return;
    }

    void updateReputation(int address, Behaviour behaviour)
    {
        map<int, Rating*>::iterator it;
        it = repTable.find(address);
        if (it == repTable.end()) {
            cout << "Adding New Entry for (repTable) "<< address <<endl;
            rating = new Rating(); 
            repTable[address] = rating;
        }
        (it->second)->updateRep(behaviour);
        cout<<"Reputation of "<<address<<"\t is ="<< (it->second)->getRep()<<endl;
    }   
};

int main() {
    int address;    
    RepSys repsys;
    while (address != 0)
    {                                
        cout << "Address\n";
        cin >> address;
        repsys.tackleFirstHandInfo(address, FORWARDING);
    }
    return 0;
}

Kind Regards Sohail

Answer 1

One of your principal problems occurs in both functions and is this:

if (it == fHandInfo.end()){
    // Some code that doesn't alter 'it'
}
(it->second)->updateRep(behaviour);

If it does point at the end then it isn't dereferencable, so the it->second has undefined behaviour. If you are going to insert something and want it to point at it then you must redo the find or use an insert method that returns an iterator (or a pair including an iterator) and re-assign it to the correct part of the return value.

Edit

A couple more points:

class RepSys {
private:
    ratingTable repTable; 
    map<int, Rating*> fHandInfo;
    Rating* rating;

You've already typedefed ratingTable to be map<int, Rating*>. It seems a bit inconsistent to use the typedef for one class variable and not for the other.

rating is a class variable, but you only seem to use it as a temporary holder in both of your functions. If this is your intended use it would be better to just use a local variable in the two functions.

You don't ever delete the Rating objects that you place in your maps. If the maps should own the Rating objects, then it would be easier from an object lifetime / memory management point of view to have a std::map<int, Rating> so that you don't have to do any manual deletion. It doesn't appear that Rating is designed to be a base call, it is a value class as it stands.

Answer 2

it = repTable.find(address);
if (it == repTable.end()){
cout << "Adding New Entry for (repTable) "<< address <<endl;
rating = new Rating(); 
repTable[address] = rating;
}
(it->second)->updateRep(behaviour);

//When address is not found in the map then after insertion the iterator it will not automatically points //to the newly inserted element. Hence, (it->second) is UB.

You can modify the code a bit:

 Rating* rating = NULL;
map<int, Rating*>::iterator it = repTable.find(address);
if (it == repTable.end())
{
 rating = new Rating(); 
repTable[address] = rating;
}
else
{
 rating = it->second;
}

rating->updateRep(behaviour);

Answer 3

Seg faults are usually because you attempt to access an object which has not been allocated.

In your code, you are calling the contructor from within an IF() block.

if (it == repTable.end()){
   cout << "Adding New Entry for (repTable) "<< address <<endl;
   rating = new Rating(); 
   repTable[address] = rating;
}

If you try to access repTable[address] and there is no object, instant exception.

Try putting it in a catch block and print out the exception details. That might give you more information as to what structure is causing a problem.

Answer 4

Slightly unrelated to the segfault, but a problem that prevents your example from working correctly : in your main function, you declare address and you don't define it before using it.

int main() {
    int address;    
    RepSys repsys;
    while (address != 0)
    {
// ...
    }

On the first run, depending on your compiler, you have no idea what the value of address is. At first, I couldn't get your example to segfault, since my compiler initialised address at 0 and it would simply skip the loop and exit.

Make sure you initialise your variables before using them.