Thread: Segmentation Fault -- Stack implementing a Linked List

The program is relatively simple, and is only suppose to take in a text file of a semicolon-delimited value/string/line, reverse the order of the values, put it all back together, and output to a new file. There are specifications, however, that we must use a stack that implements a simple Linked List data structure. I'm pretty new to C, and am just picking up the basics of pointers (I come from Java and .NET, and this is a nightmare for me to grapple with lol). Also, the specifications specifically say that the push() function MUST take in the "old top of stack (node)" and the "value to push", and return the updated pointer to the top of the stack. Also, pop() MUST take in as parameters a "pointer of a pointer to the current top of the stack", and return the value of the popped node.

We are suppose to compile with gcc -o progname progname.c -Wall -m32 -O. The source compiles just fine, but when attempting to build the stack and such with an actual sample file, I receive a Segmentation Fault. I've run my code through gdb, however, and it's saying that Segmentation Fault is occurring at my "push((values -> head), str1);" call in main(). Since this is the call to push(), I'm guessing there's more fundamental logic error I'm overlooking in my linked list and/or stack.

I've exhausted my brain at this point, and cannot for the life of me figure out where I'm going wrong. If anyone can provide me some assistance, I'd be forever grateful.

Also, if some of the code appears ugly, I apologize. I've only been working with C for about 2 weeks now

A sample input file would be:

Code:

23414;-5224;23;569;

and expected output file would be:

Code:

569;23;-5224;23414;

Here's what I've developed so far:

Code:

#include <stdio.h>
#include <assert.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdbool.h>

int STACK_ELEMENTS = 0;

typedef struct node {
    char *value;
    struct node *next;
} Node;

typedef struct my_stack {
    Node *head;
} Stack;

Stack *values;

bool stack_isEmpty() {
    if (STACK_ELEMENTS < 1)
        return true;
    else
        return false;
}

Node *push(Node *oldHead, char *val) {
    Node *newNode;
    newNode = malloc(sizeof(Node));
    newNode -> value = val;    
    
    if (oldHead == NULL) {
        values -> head = newNode;
        values -> head -> value = val;
    }

    else {
        values -> head -> next = newNode;
        values -> head = newNode;
    }
    free(newNode);
    STACK_ELEMENTS++;
    return (values -> head);
}

char *pop(Node **pHead) {
    char *val;
    if ((*pHead) != NULL) {
        Node *tempNode;
        tempNode = malloc(sizeof(Node));
        tempNode = *pHead;
        val = tempNode -> value;
        *pHead = tempNode -> next;
        free(tempNode);
        STACK_ELEMENTS--;
    }
    else
        return 0;
    return val;
    
}

int main(int argc, char *argv[]) {
    /*verify arguments*/
    if (argc != 3) {
        fprintf(stderr, "usage: strrev <file> <newfile>\n");
        exit(1);
    }
    
    char *fileIn = argv[1];
    char *fileOut = argv[2];

    /*open file streams*/    
    FILE *in = fopen(fileIn, "r");
    FILE *out = fopen(fileOut, "w");
    
    
    /*scan input file and delimt on semicolon*/
    /*input file is a single line of semicolon-delimited values*/
    char str1[128];
    while(!feof(in))
    {
        /*scan, delimiting on semicolon*/
        /*if the scanned element does not fit format, break*/
        if (fscanf(in, "%[^;];", str1) != 1) {
            break;
        }
        push((values -> head), str1);
    }
    
    while(!stack_isEmpty()) {
        fprintf(out,"%s;", pop(&(values -> head)));
    }
        
    fclose(in);
    fclose(out);
    return 0;
}

> newNode -> value = val;
This makes a copy of the pointer, not a copy of the string it points to.

Thanks for the quick reply. Would you mind explaining this a bit more for me? I'm not sure I grasp it... What I want to have happen in that call to push is to have the newNode's value field set to the passed val string. I'm sure what I'm doing is wrong, I could just use a bit more explanation, sorry.

This code is wrong, newNode -> value = val;

You need to allocate memory for newNode -> value and then copying the content from the pointer val

Code:

newNode = malloc(sizeof(Node));
if (null != newNode)
{
 newNode -> value = malloc(strlen(val) + 1);
 if (null != newNode -> value)
 {
  strcpy(newNode -> value, val);
 }
}

Also please note that you need to free the allocated pointer for the value first and then node in the function pop()

Code:

Code:

free(tempNode->value);
free(tempNode);

Oh man, thank you so much. I, too, also missed kannanm's suggestion (thank you, kannanm). Would have saved me a lot of frustration if I'd have caught it haha. Everything works as expected now.

I am still, however, struggling to understand why the simple tempNode -> value = val was not working? It seems odd to me that it would work with the old code with hard-coded string values like "hello" and "world", but not for char *str[] type values. Anyone mind helping my dumb self understand this?

Originally Posted by jackalclone1

I am still, however, struggling to understand why the simple tempNode -> value = val was not working? It seems odd to me that it would work with the old code with hard-coded string values like "hello" and "world", but not for char *str[] type values. Anyone mind helping my dumb self understand this?

It didn't work then either. You were setting the pointers to point to a previous example of a string literal, not making a copy. (Had you attempted to treat it as a copy, by say trying to change it, it would have blown up instantly.)

Some more (or more of the same) advice over here on the cross post.