Crash while working with files and linked lists

Question

I'm writing a program that takes info from a file and adds it to a linked list then and a function that adds info to the linked list. My problem is that when I call addContact() after openBook() the program crashes, but calling each of them separately works fine.

I can't find the problem.

openBook() : opens a file, reads data from file to a linkedList
addContact() : takes info, saves data to linkedList

#include <stdio.h>

typedef struct node {
    char name[50];
    char email[50];
    char ad[200];
    char phone[11];
    struct node *next;
} NODE;

NODE *head = NULL;

void openBook()
{
    FILE *read = fopen("Book.txt", "r");

    if (read == NULL)
    {
        return;
    }
    NODE *ite = NULL;
    char name[50] = "", email[50] = "", ad[200] = "", phone[11] = "";

    if (!feof(read))
    {
        head = (NODE*)malloc(sizeof(NODE));
        fscanf(read, "%s%s%s%s", head->name, head->email, head->ad, head->phone);
    }
    ite = head;
    while (!feof(read))
    {
        ite->next = (NODE*)malloc(sizeof(NODE));
        ite = ite->next;
        fscanf(read, "%s%s%s%s", ite->name, ite->email, ite->ad, ite->phone);
    }
    ite->next = NULL;
    fclose(read);
}

void addContact()
{
    NODE *ite = head;
    if (head != NULL)
    {
        while (ite->next!=NULL)
            ite = ite->next;
        ite->next = (NODE*)malloc(sizeof(NODE*));
        ite = ite->next;
    }
    else
    {
        head = (NODE*)malloc(sizeof(NODE*));
        ite = head;
    }
    fflush(stdin);
    printf("Enter name (no space): ");
    scanf("%s", ite->name);
    fflush(stdin);
    printf("Enter email : ");
    scanf("%s", ite->email);
    fflush(stdin);
    printf("Enter address : ");
    scanf("%s", ite->ad);
    fflush(stdin);
    printf("Enter phone : ");
    scanf("%s", ite->phone);
    fflush(stdin);
    ite->next = NULL;
}

void printList()
{
    NODE *iterator;
    int i;
    iterator = head;
    while (iterator != NULL)
    {
        printf("%s\n", iterator->name);
        iterator = iterator->next;
    }
}

int main()
{
    openBook();
    addContact();
    printList();
    return 0;
}

It's amazing the following works :

int main()
{
    addContact();
    printList();
    return 0;
}

but the following crashes :

int main()
{
    FILE *read = fopen("Book.txt", "r");
    fclose(read);
    addContact();
    printList();
    return 0;
}

[Don't cast `malloc()`](https://stackoverflow.com/q/605845/5958455) — iBug, Jan 03 '18 at 12:09
[Don't do `fflush(stdin)`](https://stackoverflow.com/q/2187474/5958455) — iBug, Jan 03 '18 at 12:12
[Why `while(!feof())` is a bad idea](https://stackoverflow.com/q/5431941/5958455) — iBug, Jan 03 '18 at 12:13
Your bug is most likely the fact that you may not be setting `head->next` to `NULL` upon creation. Also, listen to this guy^; **edit**: Nevermind, you are. — user123, Jan 03 '18 at 12:13
Where's your `stdlib.h`? You should have received a warning from your compiler for a declaration for `malloc()`. — iBug, Jan 03 '18 at 12:17
I copied the full code and added the missing header. Then it works fine. How? — iBug, Jan 03 '18 at 12:18
Eric Lippert's article about [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) may be worth reading. — iBug, Jan 03 '18 at 12:19
You may want to provide us your input and the content of `Book.txt`. I really can't reproduce your problem. — iBug, Jan 03 '18 at 12:21
`ite->next = (NODE*)malloc(sizeof(NODE*));` should be `ite->next = malloc(sizeof(NODE));` — Karthick, Jan 03 '18 at 14:17
why does the function: `printList()` have the local/unused variable `i`? — user3629249, Jan 05 '18 at 00:36
for flexibility, etc it is best to separate the definition of a struct from a typedef for the struct — user3629249, Jan 05 '18 at 00:37
the word: `read` is the name of a well known function in C. It is best to not use function names for variable names. — user3629249, Jan 05 '18 at 00:38
regarding: `if (read == NULL) { return; }` needs some changes. For instance, there should be a error message to `stderr` and since `fopen()` is a system function, should also output to `stderr` the reason the system thinks the function failed. Suggest: `if( ! read ) { perror( "fopen for reading 'Book.txt' failed" ); exit( EXIT_FAILURE ); }` — user3629249, Jan 05 '18 at 00:41
regarding: `if(!feof(read))` will not do what your expecting. Similar considerations exist for `while (!feof(read))` — user3629249, Jan 05 '18 at 00:43
when calling any of the heap allocation functions: (malloc, calloc, realloc, strdup) 1) always check (!=NULL) the returned value to assure the operation was successful. 2) the returned type is `void*`. It can be assigned to any pointer. Casting just clutters the code, making it more difficult to understand, debug,. etc. — user3629249, Jan 05 '18 at 00:45
when calling any of the `scanf()` family of functions, 1) always check the returned value to assure the operation was successful. 2) when using the `"%s"` or `%[...]` input format specifiers, always include a MAX CHARACTERS modifier that is one less than the length of the input buffer. This is because a NUL byte will always be appended to the input buffer. — user3629249, Jan 05 '18 at 00:47
the posted code contains some 'magic' numbers. 'magic' numbers are numbers with no basis. I.E: 11, 50, 200. 'magic' numbers make the code much more difficult to understand, debug, etc. Suggest using a `enum` statement of `#define` statements to give those 'magic' numbers meaningful names, then using those meaningful names throughout the code. — user3629249, Jan 05 '18 at 00:50
regarding this statement: `fflush( stdin );` The function: `fflush()` is ONLY for output streams, not input streams. Per the C standard, using it on an input stream is undefined behavior. (even though visual studio allows it) Suggest using a loop similar to: `int ch; while( (ch = getchar()) != EOF && '\n' != ch );` — user3629249, Jan 05 '18 at 00:54
regarding this kind of statement: `head = (NODE*)malloc(sizeof(NODE*));` This is only allocating enough room for a pointer. What is needed is to allocate enough room for a complete 'NODE'. Suggest: `head = malloc( sizeof NODE ); if( !head ) { perror( "malloc failed" ); // cleanup all/any prior allocated memory by passing the pointers to `free();` then call `exit( EXIT_FAILURE );` — user3629249, Jan 05 '18 at 01:00
regarding: `scanf("%s", ite->ad);` Most addresses have spaces, so this will not work. Suggest: `if( 1 != scanf( "%199[^\n]", ite->ad ) ) { fprintf( stderr, "trying to input the address failed\n"); // pass all/any prior memory allocations to `free(); exit( EXIT_FAILURE ); }` — user3629249, Jan 05 '18 at 01:04

score 2 · Answer 1 · answered Jan 03 '18 at 12:24

2

First of all, there are 3 points that are worth noting:

And it seems you're missing #include <stdlib.h>. You should have received a warning from your compiler for missing declaration for malloc().

After fixing the missing header, I can't reproduce your problem. I created a Book.txt and the program works perfectly fine.

This is my Books.txt for testing.

aaa aaa aaa aaa
aaa aaa aaa aaa
aaa aaa aaa aaa
aaa aaa aaa aaa
aaa aaa aaa aaa

My input used for testing:

aaa aaa aaa aaa

answered Jan 03 '18 at 12:24

iBug

35,554
7
89
134

@mohammadmozafari Yes. – iBug Jan 03 '18 at 12:29
What should i use instead of !feof()? – mohammad mozafari Jan 03 '18 at 12:30
1

@mohammadmozafari Use the return value from `fscanf()`. It reports how many items are successfully read. – iBug Jan 03 '18 at 12:32
2

worth elaborating: not having a proper declaration of `malloc()` available, and not using proper compiler options, means the compiler will assume it has the old, obsolete default of taking no arguments and returning an `int`, so extremely wrong things will happen when it's called in any other sense, and crashing is no surprise. – underscore_d Jan 03 '18 at 12:51

score 1 · Accepted Answer · answered Jan 03 '18 at 14:21

1

To add to the answers given above, one more change worth noting is ite->next = (NODE*)malloc(sizeof(NODE*)); which should be ite->next = malloc(sizeof(NODE)); You have just done malloc for the size of the pointer NODE* but you need to malloc for the size of the NODE

answered Jan 03 '18 at 14:21

Karthick

1,010
1
8
24

I think that's it (although using `malloc()` with cast and without the proper includes is another good candidate) – Ingo Leonhardt Jan 03 '18 at 17:52

Crash while working with files and linked lists

2 Answers2