Stack implementation in C++ using a class causing segmentation fault

Question

I have implemented some functions for a Stack in C++. I am unsure as to why I am getting a segmentation fault. Right now, I have 7 different files: node.h, node.cpp, LL.h, LL.cpp, Stack.h, Stack.cpp and main.cpp which I am using to test LL.cpp and Stack.cpp. If anyone could direct me to the error, I would really appreciate it. Here are the codes:

node.h :

// node.h

class node { // node class used in the LL (linked list) class
    private:
        node * next; // Pointer to next node of an LL
        int data;    // integer data stored in this node

    public:
        node(int x, node * n);  // Constructor
        ~node();                // Destructor
        void set_data(int x);   // Change the data of this node
        void set_next(node * n);// Change the next pointer of this node
        int get_data();         // Access the data of this node
        node * get_next();      // Access the next pointer of this node
};

LL.h :

// LL.h
#include "node.h"

// Linked list class, used in the Stack class
class LL {
    private:
        node * head; // pointer to first node
        node * tail; // pointer to last node

    public:
        LL(); // Constructor
        ~LL(); // Destructor
        void prepend(int value); // add a node to the beginning of the LL
        int removeHead();        // remove the first node of the LL
        void print();            // print the elements of the LL
    node * get_head();       // access the pointer to the first node of the LL
};

Stack.h:

// Stack.h
#include "LL.h"

class Stack {
    private:
        LL * intlist;

    public:

        Stack();    // Constructor
        ~Stack();       // Destructor

        void push(int value);
        int pop();      
        int isEmpty();
        void Sprint();
};

Stack.cpp:

// Stack.cpp
#include "Stack.h"
#include <stdio.h>

Stack::Stack() {
}

Stack::~Stack() {
}

int Stack::isEmpty() {
    return ( (intlist->get_head()) ==NULL);
}

void Stack::push(int value) {
    intlist->prepend(value);
}

int Stack::pop() {

    if ( ! isEmpty() ) {
        int result=intlist->removeHead();
        return result;
    }
    return -1;
}

void Stack::Sprint() {

    intlist->print();
}

And here is the main.cpp that I am using to test it:

// main.cpp
#include "Stack.h"
#include <stdio.h>

int main() {
        LL a;
        a.prepend(3);
        a.prepend(4);
        a.prepend(5);
        a.print();
        a.removeHead();
        a.print();

        Stack sta;
    sta.pop();
        sta.push(3);
        sta.push(4);
        sta.push(10);
    sta.Sprint();

    printf("Popping %d\n", sta.pop());
        sta.Sprint();

        sta.pop();
    printf("Stack empty? %d\n", sta.isEmpty());

        sta.pop();
        printf("Stack empty? %d\n", sta.isEmpty());
    return 0;
}

I have been trying to find what's causing the segmentation fault for a while. Any help appreciated

The best way to find out why the program is crashing is to remove from the program everything that isn't crashing and examine what remains. See [mcve] for advice on how to do this. — user4581301, Mar 15 '19 at 22:07
@user4581301 Actually, the best way to determine why a program is crashing is to run it with a debugger which will break on error. So you should try doing that. I'm also somewhat confident it's crashing because you're calling delete on pointers that down actually own any memory. If you haven't called new, you shouldn't call delete. Additionally you should work on improving your coding style and using C++ stuff instead of C stuff, like iostream and nullptr. — user8145466, Mar 15 '19 at 22:13
I did try commenting out everything such as as sta.pop(), sta.push, sta.print() but I still got the Seg fault. I am thinking the error might be in the declaration in the line that says "Stack sta" ? But unsure as to why that might be the case. Anyway, thanks for the tip — john, Mar 15 '19 at 22:14
My bad about the new/delete, I glanced and didn't see you actually were calling new. — user8145466, Mar 15 '19 at 22:15
Shouled `Stack::Stack() {}` not be setting up a linked list? I recommend not using a pointer to a `LinkedList`, by the way. The `Stack` is going to contain one and only one `LinkedList` and I doubt polymorphism is going to come into play, so `LL intlist;` makes better sence than `LL * intlist;` — user4581301, Mar 15 '19 at 22:21
@john it is up to you, but the way it is, it can have any value. But, I guess the first question is: is it really necessary to be a pointer? — Amadeus, Mar 15 '19 at 22:21
In addition, keep an eye out for [The Rule Of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — user4581301, Mar 15 '19 at 22:22

Alecto Irene Perez · Accepted Answer · 2019-03-15T23:26:21.800

This program is crashing because your Stack class never initializes the Linked List pointer (LL * intlist), so when you check if it's empty it tries referencing garbage:

Stack.pop() => Stack.isEmpty() => intlist->isEmpty() => segfault

You could just make it a member instead of a pointer (recommended):

class Stack {
   private:
    LL intlist; // There's no need for it to be a pointer

   public:

    Stack();    // Constructor
    ~Stack();       // Destructor

    void push(int value);
    int pop();      
    int isEmpty();
    void Sprint();
};

Alternatively, you could initialize it in the constructor and delete it in the destructor. You should really only do this when a member variable needs to be a pointer; otherwise you should just store it regularly.

Stack::Stack() : intlist(new LL()) {       
}

Stack::~Stack() {
    delete intlist; 
}

Absolutely! Leave a comment if you have any more questions or issues! — Alecto Irene Perez, Mar 16 '19 at 00:19

score 2 · Answer 2 · answered Mar 15 '19 at 22:27

Pointer intlist in Stack is never initialized, so attempting to dereference it caused your segfault (in this case, it was when sta.pop() was first called).

You could allocate memory for intlist in Stack's constructor (e.g. intlist = new LL;) and delete it when you're done (or use a smart pointer instead). But in this case that'll do more harm than good.

You're likely better off defining intlist as an object of type LL, not a pointer to it, in Stack:

class Stack {
    private:
        LL intlist;

    ...
};

Of course, don't forget to replace all arrow operators (->) with dot operators (.) when working with intlist.

Stack implementation in C++ using a class causing segmentation fault

2 Answers2