c++ segmentation fault after creating a point of a class

Question

I have a c++ code segment that throws segmentation Error:

Matrix representation of graph

0100

0010

1011

0000

Node, label:1,column index:0

/C/Program Files/NetBeans 8.1/ide/bin/nativeexecution/dorun.sh: line 33: 13140 Segmentation fault sh "${SHFILE}"

code snippet:

     void graph::shortest_path(std::vector<std::vector<int>> &_matrix) {
    //1) initialize not visited nodes 
    int num_of_nodes = _matrix.size();
    std::queue<Node> unvisited;
    std::queue<Node> visited;

    for (int i = 0; i < num_of_nodes; i++) {
        unvisited.push(Node(i + 1));
    }



    //while unvisited is NOT empty
    while (!unvisited.empty()) {


        //2) pop/remove from unvisited & are there adjacent neighbors 
        Node node = unvisited.front();
        std::cout << "Node, label:" << node.getLabel() << ",column index:" << node.getLabel() - 1 << endl;
        vector<int>& columnVector = _matrix[node.getLabel() - 1];
        //nodes integer label 
        int label = 0;
        //loop add adjacency list * back pointers
        for (int c = 0; c < columnVector.size(); c++) {
            //if there are actual connections, then adjacency matrix value at C NOT equal INT_MAX
            if (columnVector[c] != INT_MAX) {
                //create a node & add to current nodes adjacency list 
                Node * adj = new Node(c+1); 
                adj->setPrev(node);
                node.addToAdjacenyList(*adj);
                //also set the prev reference 

            }
        }//end loop add adjacency list * back pointers


        //3) for each node calculate the total weight or cost back to the start & select the min 
        for (int i = 0; i < node.getAdjacenyList()->size(); i++) {
            cout << node.getAdjacenyList()->at(i).getLabel() << ",";


        }
        cout << endl;

        //keep track of visited nodes 
        visited.push(node);
        //since this node was visited remove/pop it from unvisited 
        unvisited.pop();


    }

}

However when I just instantiate without the new keyword I dont get any segmentation fault:

//loop add adjacency list * back pointers
        for (int c =0; c<columnVector.size(); c++)
        {
            //if there are actual connections, then adjacency matrix value at C NOT equal INT_MAX
            if (columnVector[c]!=INT_MAX)
            {
                //create a node & add to current nodes adjacency list 
                //Node * adj = new Node(c+1); 
                //adj->setPrev(node);
                node.addToAdjacenyList(Node(c+1));
                //also set the prev reference 

            }
        }//end loop add adjacency list * back pointers

Ok Output:

Matrix representation of graph

0100

0010

1011

0000

Node, label:1,column index:0 2, Node, label:2,column index:1 3, Node, label:3,column index:2 1,3,4, Node, label:4,column index:3

**Here is the Node class:**

/*
 * To change this license header, choose License Headers in Project Properties.
 * To change this template file, choose Tools | Templates
 * and open the template in the editor.
 */

#include "Node.h"
#include <vector>

Node::Node()
{

}

Node::Node(int label):_label(label)
{

}

Node::~Node()
{
    Node::_adjacency_list->clear(); 
}

void Node::setLabel(int val)
{
    _label = val; 
}

int Node::getLabel()
{
    return _label; 
}

void Node::setWeight(int val)
{
    _weight = val; 
}

int Node::getWeight()
{
    return _weight; 
}

int Node::getDistance()
{
    return _distance;
}

void Node::setDistance(int val)
{
    _distance = val; 
}

/*
 * std::vector<Node>* getAdjacenyList();
    void addToAdjacenyList(const Node& node);
 */

std::vector<Node>* Node::getAdjacenyList()
{
    std::vector<Node>* point = &*_adjacency_list;
    return point; 
}

void Node::addToAdjacenyList(const Node& node)
{
    _adjacency_list->push_back(node);
/*
 * 
 *  Node* getPrev();
    void  setPrev(const Node& node); 
 */
}

Node* Node::getPrev()
{
     Node* point = &*_prev;
     return point; 
}

void Node::setPrev(const Node& n)
{
    *_prev = n; 
}

Why am I getting segmentation fault & how can I fix it?

Thanks

post a [minimal, complete, verifiable example](https://stackoverflow.com/help/mcve) — Alan Birtles, Mar 23 '18 at 18:31
`{ Node * adj = new Node(c+1); adj->setPrev(node); node.addToAdjacenyList(*adj); }` -- Memory leak. — PaulMcKenzie, Mar 23 '18 at 18:45
@PaulMcKenzie, why is that a memory leak. Pointer of type Node is instantiated using new Node(c+1). Then its accessed. Please elaborate. — cyber101, Mar 23 '18 at 18:56
How are you going to `delete` that Node pointer when that pointer goes out of scope? Do you save that pointer value somewhere for later `delete`-tion? — PaulMcKenzie, Mar 23 '18 at 18:59
@Im not finished coding ,inside the ~Node destructor, I will iterate through the Nodes adjacency list and call delete. Actually in the ~Node() im calling adjacency_list->clear(); — cyber101, Mar 23 '18 at 19:01
@cyber101 -- You have a clear misunderstanding of what `new` does in `C++`. C++ is not Java. What pointers are you saving? This: `node.addToAdjacenyList(*adj);` does not use or save any pointers. — PaulMcKenzie, Mar 23 '18 at 19:02
@cyber101 -- It is very simple. The `new` in C++ returns a pointer value to you. You must save this value so that later on, you can call `delete`. You declared a *local* `adj` pointer. It got assigned a value by `new`. You then go out-of-scope, losing the pointer and the value it was assigned. Memory leak. What you probably are being confused by is `*adj` -- That is *not* a pointer -- it is an object. — PaulMcKenzie, Mar 23 '18 at 19:07
@PaulMcKenzie 1) When new is used to allocate memory for a C++ class object, the object's constructor is called after the memory is allocated. That what I understand. 2) Yes C++ is NOT Java and Java is NOT C++, so what. 3) node.addToAdjacenyList(*adj) is NOT saving a pointer this dereferencing a pointer which passes pointer address to Node& node reference variable. Again how can I fix the code? — cyber101, Mar 23 '18 at 19:08
The fix: No `new`. `Node adj(c+1);` allocates and constructs an Automatic variable. It's copied into `_adjacency_list` by `push_back`, and the original goes out of scope and destructs. From there `_adjacency_list` manages all of the storage for you. — user4581301, Mar 23 '18 at 19:10
A note about `Node::_adjacency_list->clear();`: `vector` automatically clears for you on destruction so you don't need to do this (See [The Rule of Zero](http://en.cppreference.com/w/cpp/language/rule_of_three)) and you shouldn't need to prefix with the class name. If you get an error if you don't use the classname, likely you have another bug lurking somewhere. — user4581301, Mar 23 '18 at 19:15
@cyber101 -- So you finally admit it is a memory leak? The reason why I mention Java is that code that has `new` all over the place without a single `delete` is a tell-tale sign of Java-like coding going on in a C++ program. Second, you get less issues with code that reduces or eliminates the usage of pointers. Segmentation faults are an indication of pointer mismanagement. — PaulMcKenzie, Mar 23 '18 at 19:17
An unrelated note on using underscore prefixes: Underscores often mean something in C++. Use them with caution. More on the subject here: [What are the rules about using an underscore in a C++ identifier?](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier) — user4581301, Mar 23 '18 at 19:33

user4581301 · Answer 1 · 2018-03-23T19:35:49.610

void Node::setPrev(const Node& n)
{
    *_prev = n; 
}

_prev is never pointed at a valid memory location before dereferencing it and copying n into it. Dereferencing an invalid pointer invokes dreaded Undefined Behaviour. Copying into invalid memory is also Undefined Behaviour and is likely what's resulting in the Segmentation fault.

Probably what you need intend is

void Node::setPrev(Node * n)
{
    _prev = n; 
}

But this will likely set off a potentially nasty chain of events elsewhere because the adjacency list is also storing copies. A significant redesign is in order.

There is an immediate echo in

Node* Node::getPrev()
{
     Node* point = &*_prev;
     return point; 
}

&*_prev; dereferences the pointer and promptly takes the address again. Net result is doing nothing useful if _prev is pointing at valid memory. If _prev is not pointing at valid memory, and it isn't, the result of *_prev is undefined.

Node* Node::getPrev()
{
     return _prev; 
}

is safer, but places the onus on the receiver to make sure what it received is valid before attempting to use it.

c++ segmentation fault after creating a point of a class

1 Answers1