Segfault when assigning one pointer to another

Question

My brain has never really quite grasped linked lists and the finer points of pointers but I'm trying to help out a friend with some C++ assignments. (And before I go any further, yes, there is std::list but I'm looking for an academic answer, and maybe something that will make linked lists more understandable to he and myself).

What we need to do is generate a linked list of objects (a Employee object) based on user input, and then display that information back to the user. Whenever I try to assign the object into the Linked List Container, it segfaults.

I have the following Linked List object:

class LinkedListContainer {
    private:
        Employee *emp;
        LinkedListContainer *next;

    public:
        Employee getEmployee() { return *emp; }

        void setEmployee(Employee *newEmp) {
            *emp = *newEmp // This is what is causing the segfault
        }

        LinkedListContainer getNext() { return *next; }

        void setNext(LinkedListContainer *newContainer) {
            *next = *newContainer;
        }
}

I'm sure that I'm doing something horribly wrong.

Answer 1

Looking at your class, there doesn't appear to be a place where the pointer emp is set to point at an actual object.

This line:

*emp = *newEmp;

assigns the value of the object pointed to by newEmp to the object pointed to by emp. Unless both pointers point at valid objects, the code will have undefined behaviour.

You may be better having emp as an Employee object rather than as a pointer to an object requiring manually management of the pointed to object's lifetime.

This assumes that your LinkedListContainer class is a node which will own the Employee.

On the other hand when you do:

*next = *newContainer;

from the naming I would assume that you just want to point the next pointer at another LinkedListContainer for which you would probably want to do:

next = newContainer;

as this assigns the value of the pointer to the variable next.

You need to be clear when you design your class and use pointers, on which objects own which other objects and ensure that you manage their lifetimes appropriately.

Answer 2

*emp = *newEmp

You don't want to do this - in fact, you don't want to dereference any pointers at all here.

emp = newEmp

Answer 3

By default, emp is a pointer that's pointing nowhere. By writing

*emp = *newEmp;

you try to assign the content of newEmp to whatever memory location is pointed to by emp. As said, emp is pointing nowhere, so you are dereferencing a NULL pointer here, which is causing the segmentation fault.

If your container is to contain a complete Employee, you'd better declare emp to be of type Employee (not pointer to Employee as now). Then

emp = *newEmp;

will work, although I'm not quite sure whether this will be all you'd need to fix about your LinkedListContainer.

Answer 4

*emp = *newEmp;

Should be:

emp = newEmp;

So that you're assigning the pointer and not the object pointed to by the pointer.

Answer 5

Your emp pointer is uninitialized, so when you attempt to dereference it (*emp) in setEmployee() you attempt to access memory that doesn't belong to you (hence the segfault).

You might be better off holding the Employee by value (assuming it's not polymorphic) and passing the setEmployee an Employee object by const reference:

class LinkedListContainer {
  Employee emp;

  // ...

  void setEmployee(const Employee& newEmp) {
    emp = newEmp;
  }

  // ...
};

Of course, you'll need to adjust your other member functions as well to reflect using a value vs. a pointer.

Good luck!

Answer 6

One of the issues that you are having is because of the leading * when you access the pointer. What * tells the compiler when accessing a pointer is that instead of reading the address the pointer points to it should read the value of the location that the pointer points to.

An example is variables are like houses that hold values. A pointer is like the address on the house. Typically when the compiler reads a pointer it only sees the address. When you put * infront of the pointer it tells the compiler to look inside the "house" to extract the value within. When you assign pointers new addresses you do not want to use the * or else you are copying values and not the addresses.

So what you want to be doing instead is in the setNext for example:

next = newContainer;

Answer 7

Previous answers explain reason of segmentation fault. Anyway if you need that sample for academic use, then IMHO you forgot about initialization of class members. The second issue is memory management - who does alloc/free these objects? In your sample there is nor constructor, neither destructor :)

Your class could look like this one below:

class LinkedListContainer 
{    
    public:        
        LinkedListContainer()
            : d_emp( 0 ) 
            , d_next( 0 )
        {
        }

        bool isValid() const
        {
            const bool result = ( d_emp != 0 );
            return result;
        }

        const Employee& getEmployee() const
        { 
            assert( isValid() );
            return *d_emp; 
        }        

        void setEmployee( Employee* emp ) 
        {            
            d_emp = emp;
        }        

        LinkedListContainer* getNext() 
        { 
            return d_next; 
        }        

        void setNext( LinkedListContainer* next )
        {            
            d_next = next;        
        }

    private:        
        Employee* d_emp;        
        LinkedListContainer* d_next;    

};

If you don't want to bother with memory management for Employee objects, then just use shared_ptr from boost library.

typedef boost::shared_ptr< Employee > SmartEmployee;

class LinkedListContainer 
{    
    public:        
        LinkedListContainer()
            : d_next( 0 )
        {
        }

        bool isValid() const
        {
            return d_emp;
        }

        const Employee& getEmployee() const
        { 
            assert( isValid() );
            return *d_emp; 
        }        

        void setEmployee( SmartEmployee emp ) 
        {            
            d_emp = emp;
        }        

        LinkedListContainer* getNext() 
        { 
            return d_next; 
        }        

        void setNext( LinkedListContainer* next )
        {            
            d_next = next;        
        }

    private:        
        SmartEmployee d_emp;        
        LinkedListContainer* d_next;    

};

Answer 8

If you want to expand the capabilities of your linked list without using templates, I suggest creating a node class and let your clients inherit from it:

class Node_Interface
{
  public:
    Node_Interface()
      : m_next(NULL)
    { ; }

    Node_Interface * get_next(void) const
    { return m_next;}

    void link_to_next(Node_Interface * p_node)
    { m_next = p_node;}

    virtual bool equal_to(Node_Interface * p_node) const = 0;
    virtual bool less_than(Node_interface * p_node) const = 0;
  private:
    Node_Interface * m_next;
};

class Employee
  : public Node_Interface
{
  public:
    Employee(const std::string new_name = std::string("unknown"))
    : Node_Interface(),
      m_name(new_name)
    { ; }
    bool equal_to(Node_Interface * p_node) const
    {
      if (!p_node) return false;
      Employee * p_employee = dynamic_cast<Employee *>(p_node);
      return m_name == p_employee->m_name;
    }
    bool less_than(Node_Interface * p_node) const
    {
      if (!p_node) return false;
      Employee * p_employee = dynamic_cast<Employee *>(p_node);
      return m_name < p_employee->m_name;
    }
};

Now the linked list can accept anything derived from Node_Interface. The pure virtual methods, equal_to and less_than, allow the linked list to compare nodes and let the descendant define how the comparison is performed.