strcpy / strncmp segmentation fault in struct fileds

Question

I'm trying to add a new node to a struct with a char* field (word)

Definition of listT:

enum boolean {false, true};
struct list {
    enum boolean sorted;
    union{
        int words;
        char *word;
    };
    struct list* next;
    struct list* previous;
};
typedef struct list listT;

add_word_node function is being called by main as: add_word_node(read_word, list_head) where read_word is given by the user with scanf. Word is being pass as a string but after strncpy there is no terminating byte.

>Debugger:
     add_word_node (word=0xbffff0fa "ally", head=0x804c038) at prog.c    
>Debugger:
     (gdb) p newnode->word


     $2 = 0x804c068 "allyP\224\373\267\377\377\377\377" 




 listT *add_word_node(char word[], listT *head) {
    listT *newnode;
    listT *curr = NULL;//sorted
    //listT *prev;//sorted

    newnode = malloc(sizeof(listT)); /* allocate new node and check */
    newnode->word = malloc(sizeof(char)* WORDLEN);
    strncpy(newnode->word, word, strlen(word));
    //newnode->word = strndup(word, strlen(word));


    if (newnode == NULL) {
        return (NULL);
    }

    if (head->sorted == false){ //eisagwgh sto telos ths listas
        newnode->next = head;
        head->previous->next = newnode;
        newnode->previous = head->previous;
        head->previous = newnode;
    }else {
        if(head->next == head){
            newnode->next = head;
            newnode->previous = head;
            head->next = newnode;
            head->previous = newnode;

        }else{
            for (curr = head->next; curr->next != NULL; curr = curr->next){//for( curr = head->next; ;){
                if(curr == NULL) break;
                if(strncmp(curr->word,newnode->word,strlen(word)) > 0) break;
                //else curr= curr->next;
            }
            if(strncmp(curr->word,newnode->word,strlen(word))== 0){
                return(curr);
            }
            newnode->next = curr;
            newnode->previous = curr->previous;
            newnode->previous->next = newnode;
            newnode->next->previous = newnode;
        }
    }

    return (newnode);

}

I have read some other topics about this problem and I changed the function to use word[] instead of char* but it still doesn't work. Please tell me if you need more info. Also, when I use strndup, it sometimes work without any error.

Are you sure when `curr = head->next` that head->next is not NULL? Because when that is the case curr->word will give seg fault for sure.. Yes that is the problem. — Igor Pejic, Dec 27 '14 at 17:49
I added an if statement to break the for loop in case curr == NULL but still get the same seg fault. — user3163175, Dec 27 '14 at 17:57
In your `for` loop, I'd check the validity of `curr`, and not just reassign it in an `else`: `for (cur = head->next; curr->next; curr = curr->next)` would be a little safer — Elias Van Ootegem, Dec 27 '14 at 18:05
I changed as you suggested but I still get seg. `for (curr = head->next; curr->next != NULL; curr = curr->next) if(curr == NULL) break; if(strncmp(curr->word,newnode->word,strlen(word)) > 0) break;` — user3163175, Dec 27 '14 at 18:12
`enum boolean {false, true}; struct list { enum boolean sorted; union{ int words; char *word; }; struct list* next; struct list* previous; }; typedef struct list listT;` — user3163175, Dec 27 '14 at 18:19
Function `add_word_node` is never called, so nothing could possibly happen in this program. In addition, structure `listT` is not defined, so this code cannot even compile. — barak manos, Dec 27 '14 at 18:43
@user3163175: it is false safety. It does exactly the same thing as `strcpy(newnode->word, word);` and offers no additional "protection" (strncpy never does) Also the definition for your `List` structure is missing. (is newnode->word a pointer or an array? it could even be a Variable Length Array) — wildplasser, Dec 27 '14 at 18:47
@barak manos listT is defined and I posted it 3 comments above. Also this is a function of a program not a single program. — user3163175, Dec 27 '14 at 18:48
Well first of all, post the relevant information within the question, not in comments. Second, it didn't occur to me that this function was your entire program. I was trying to politely imply that you have not posted all the relevant information required in order to answer your question. You should at least show what parameters you are calling this function with. — barak manos, Dec 27 '14 at 18:50
@barakmanos I hope I didn't offend you (I didn't mean to). You are right and I apologizing for not being clear enough i will add more info. — user3163175, Dec 27 '14 at 18:53

Gopi · Accepted Answer · 2014-12-27T18:45:02.603

0

As you say you have a char *word then this pointer should be allocated memory

newnode->word = malloc(sizeof(char) * (WORDLEN+1)); /* Please access the structure elements accordingly */

Memory should be allocated to the pointer before writing something to it.

edited Dec 27 '14 at 18:45

answered Dec 27 '14 at 18:20

Gopi

19,784
4
24
36

Doens't this: `newnode = (listT *) malloc(sizeof(listT) + WORDLEN);` allocate memory for char* word? – user3163175 Dec 27 '14 at 18:24
@user3163175 I can't see what is WORDLEN in the code but it is not needed just allocate memory to your struct and allocated memory the pointer withing the structure – Gopi Dec 27 '14 at 18:25
Thanks for the info. I changed it but I don't think that this solves the problem since WORDLEN wach bigger than the actual length of words. – user3163175 Dec 27 '14 at 18:29
@user3163175 PLease allocate memory to hold the `WORDLEN` Check edits – Gopi Dec 27 '14 at 18:32
I did but nothing changed – user3163175 Dec 27 '14 at 18:39
@user3163175 Do you still see segmentation fault? `WORDLEN + 1` to hold the NULL terminator? – Gopi Dec 27 '14 at 18:45
I was wrong. I changed malloc but now the word is being passed as i.e. "alpha" but there is no \0 after strncpy – user3163175 Dec 27 '14 at 19:09
I added an example in question – user3163175 Dec 27 '14 at 19:18
@user3163175 You NULL terminate it manually. Add `memset(newnode->word,'\0',(WORDLEN+1))` – Gopi Dec 27 '14 at 19:22
Memset wasn't working as I wanted (It added \0 at the begginig) but since word is an array I just added it in the end. Thank you so much for your time and help. – user3163175 Dec 27 '14 at 20:25

score 0 · Answer 2 · edited May 23 '17 at 12:12

To expand @Gopi's answer a bit:

Your statement

newnode = (listT *) malloc(sizeof(listT) + WORDLEN);

allocates memory to the pointer newnode. This means that, for example if WORDLEN is the same as sizeof(listT), then you will be allocating memory for two listT elements for newnode. This is similar to, say, char *ptr = malloc(2); which will allocate memory for two char elements for ptr.

TL;DR: This WORDLEN memory will NOT be allocated to newnode->word, it is allocated to newnode, and that's that. To fix your problem, you must allocate memory to newnode->word separately. newnode does not need that memory, but newnode->word does.

strndup() works because it allocates the memory for you.

If you still struggle to understand this, you may have some misconceptions on pointers.

newnode->word is a pointer. It has a fixed size on a certain machine (e.g. 8 bytes). For simplicity assume listT is defined as follows:

typedef struct {
    char *word;
} listT;

Then when you allocate memory, you only need to allocate 8 bytes to newnode so that it can hold one pointer, and then you need to allocate memory to that pointer, which will be used to store some characters.

Although not related to your problem, I would also like to point out that you shouldn't cast the return value of malloc(). See Do I cast the result of malloc?

strcpy / strncmp segmentation fault in struct fileds

2 Answers2