Linked List in C not working Valgrind Error

Question

The program is supposed to read the words of a file to a linked list. I got the Linked list working but now I'm faced with another error. I'm getting a segmentation fault when using a very large file.

It is working perfectly for smaller lists (3-10 words) but not for larger ones.

-----CODE-----
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct node //struct for linked list
{
    char* word;
    struct node* next;
}
node;  

int main (void)
{        
    char* dictfile = "large";

    FILE* dict = fopen(dictfile, "r");

    if (dict == NULL)
        return 1;

    node* head = NULL;       

    char* oneword = malloc(sizeof(node));  //to store the word from fgets()

    while ((fscanf (dict, "%s", oneword)) > 0)
    {                                        
        node* temp = (node*)malloc(sizeof(node));

        char* tempword = (char*)malloc(sizeof(node)); //gives me a new pointer to store the string (as pointed out by Antione)

        strcpy(tempword, oneword);

        temp->word = tempword;

        printf("%s\n", temp->word);     //prints the value (just for debug)

        temp->next = head;            
        head = temp;             
    }

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

    while(temp != NULL)    //loop to print the linked list
    {           
        printf("traverse %s\n", temp->word);    //prefix 'ing traverse to know its a linked list
        temp = temp->next;
    }

    free(head);
    free(temp);
    fclose(dict);

    printf("SUCCESS!!\n");      

}

The result is as follows:

./tempeol
.............lots of words then the below
acceptor
acceptor's
tempeol: malloc.c:2372: sysmalloc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 *(sizeof(size_t))) - 1)) & ~((2 *(sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long) old_end & pagemask) == 0)' failed.
Aborted (core dumped)

Valgrind shows invalid writes at a few places.

One is where for the strcpy(). I have to account for the additional null character to resolve this but I don't know how.

    -----Valgrind Error-----
jharvard@appliance (~/Dropbox/pset5): valgrind ./tempeol
==4709== Memcheck, a memory error detector
==4709== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==4709== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==4709== Command: ./tempeol
==4709== 
a
aaa
aaas
aachen
aalborg
==4709== Invalid write of size 1
==4709==    at 0x40DB401: _IO_vfscanf (vfscanf.c:1180)
==4709==    by 0x40E21F6: __isoc99_fscanf (isoc99_fscanf.c:34)
==4709==    by 0x8048681: main (tempeol.c:25)
==4709==  Address 0x423b1c0 is 0 bytes after a block of size 8 alloc'd
==4709==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==4709==    by 0x8048662: main (tempeol.c:23)
==4709== 
==4709== Invalid read of size 1
==4709==    at 0x402D4F1: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==4709==    by 0x80486D5: main (tempeol.c:31)
==4709==  Address 0x423b1c0 is 0 bytes after a block of size 8 alloc'd
==4709==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==4709==    by 0x8048662: main (tempeol.c:23)
==4709== 
aalesund

Help needed!!

Also a funny thing: if I change char* tempword = (char*)malloc(sizeof(node)); to char* tempword = (char*)malloc(sizeof(node)*512); I end up getting the full linked list with a segmentation fault at the end.

I feel the solution lies in this line of code. Any ideas??

===============================================================

Thanks to @DrC and @lurker, I've got it working. Valgrind shows no errors. No segmentation faults. :)

Here's the final working code for anyone who needs it:

================WORKING CODE:: SOLUTION============
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct node //struct for linked list
{
    char* word;
    struct node* next;
}
node;  

int main (void)
{        
    char* dictfile = "small";

    FILE* dict = fopen(dictfile, "r");

    if (dict == NULL)
        return 1;

    node* head = NULL;       

    char oneword[28];  //to store the word from fscanf()

    while ((fscanf (dict, "%s", oneword)) > 0)
    {                                        
        node* temp = (node*)malloc(sizeof(node));

        char* tempword = (char*)malloc(strlen(oneword)+1); //gives me a new pointer to store the string (as pointed out by Antione)

        strcpy(tempword, oneword);

        temp->word = tempword;

        //printf("%s\n", temp->word);     //prints the value (just for debug)

        temp->next = head;            
        head = temp;             
    }

    node* temp = head;

    while(temp != NULL)    //loop to print the linked list
    {           
        printf("%s\n", temp->word);    //prefix 'ing traverse to know its a linked list
        temp = temp->next;
    }

    printf("\n");

    free(head);
    //free(temp);   //no need for this (DrC)
    fclose(dict);

    printf("SUCCESS!!\n");      

}

I can sleep now...

You should [not cast `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). Why is a `tempword` being allocated the size of a `node`? Are you sure `fscanf` is only reading in words that are 8 bytes (in a 32-bit system) or 16 bytes (in a 64-bit system) long, including zero termination? That's how large a `node` is. You're probably just lucky it's working for a small number of words and not overwriting anything that's hurting your program (yet). — lurker, Mar 20 '15 at 21:03
Thanks @lurker. To be honest idk. (I had tried compiling with sizeof(char), then tried sizeof(node). Neither worked so I didn't change it). Idk about fscanf reading 8/16 bytes. How to check that?? — jzz, Mar 20 '15 at 23:33
First it will be important to know what the longest word is that you might read in. Then you can allocate an amount of memory that will hold it. — lurker, Mar 20 '15 at 23:46
I'm loading a dictionary. The longest word in the dictionary is 45 characters. So malloc(sizeof(char)*45)? (I got the rest of the code working, it was indeed the malloc causing errors. — jzz, Mar 21 '15 at 00:00

DrC · Accepted Answer · 2015-03-20T21:04:12.077

1

You have the correct line of code for the first problem. You malloc the memory to hold the string based on sizeof(node). Instead, it should be of length strlen(oneword)+1. Right now, any long word will write beyond the buffer.

char* tempword = (char*)malloc(strlen(oneword)+1);

In the lines:

...}

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

while(temp != NULL)  ...

you assign temp to some malloc'd storage and then immediately set temp to head - thus leaking the storage (minor).

Very minor item - the call to free(temp) at the end is a waste as temp is necessarily null.

edited Mar 20 '15 at 21:04

answered Mar 20 '15 at 20:58

DrC

7,528
1
22
37

glad to say, it is NOW working! Never thought two lines of code were breaking the entire program. Now valgrind shows clean. This is what I did. I changed `char* oneword = malloc(sizeof(node));` to `char oneword[28];` and `char* tempword = (char*)malloc(sizeof(node))` to `char* tempword = (char*)malloc(strlen(oneword)+1);`. Thanks again. – jzz Mar 20 '15 at 23:48

Linked List in C not working Valgrind Error

1 Answers1