CODEWITHSUNDEEP
c
studentLOL
studentLOL

Reputation: 35

Segmentation fault with strncpy

EDIT: Variable names

I'm making a linked list, and when I try to free a node it gives me an error. I tracked my code and found that my error is rooted when I create a node with this code.

The weird part is that if I assign one char less than what I want it to be it works. Also, it works fine for assigning "word", the issue lays in "id".

struct node* makeNewNode (char* word, char* id, int occurrences) {
    //make space for the new node
    struct node* temp = malloc (sizeof(struct node*));

    temp->word = malloc(sizeof(char) * strlen(word) + 1);
    strncpy(temp->word, word, strlen(word));
    temp->word[strlen(word)] = '\0';


    temp->id = malloc(sizeof(char) * strlen(id) + 1);
    strncpy(temp->id, id, strlen(id));
    temp->id[strlen(id)] = '\0';

    //assign the number of occurrences to the argument passed in
    temp->occurrences = occurrences;

    //return the newly created node
    return temp;

}

The struct for the node is:

struct node {
        char* word;
        char* id;
        int occurrences;
        struct node* next;
};

What I mean by one less is that this works:

strncpy(temp->id, id, strlen(id)-1);

However it means that I am losing one char consistently.

I've tried to manually copy the string with a for loop but it also doesn't work. I've tried appending one '\0' char but it also doesn't work.

If needed I can provide what I'm using to test this

Upvotes: 3

Views: 102

Answers (2)

Carl Norum
Carl Norum

Reputation: 224864

The likely candidate is this line:

struct node* temp = malloc (sizeof(struct node*));

Which creates enough space to store a pointer to a node, not to a node itself. Remove the * from the sizeof expression. Alternatively (and the way I would write this code), just don't use types in sizeof expressions if you can avoid it:

struct node *temp= malloc(sizeof *temp);

Other notes:

  1. As mentioned by @VladFeinstein, use strdup instead of your malloc/strlen/strncpy/\0 dance.

    temp->word = strdup(word);
temp->id = strdup(id);

  2. If you choose to not do that, notice that your order of operations seems confused in the malloc size expressions:

    temp->word = malloc(sizeof(char) * strlen(word) + 1);

    It's still correct, but only because sizeof(char) is 1. I'd simply write:

    temp->word = malloc(strlen(word) + 1);

    But if you're really set on leaving sizeof(char) in there, make sure you parenthesize the addition in the expression correctly.

Upvotes: 5

Vlad Feinstein
Vlad Feinstein

Reputation: 11311

We can look for an off by 1 error in your code.

Alternatively, you can replace the use of malloc, strncpy and adding \0 with one call to strdup.

Upvotes: 0

Related Questions