Run-time crash in C program mimicking 'strings' UNIX command

Question

Follow up of the original program: Segmentation fault (core dumped) in C byte reader

My program is supposed to sort of mimic (in a primitive way), but currently isn't working.

I've been working on this for a decent amount of time, and previously I was getting a segmentation fault (core dumped) due to an uninitialized node that I use as the first_node, or because of the way I had previously used feof() instead of fread(...) != 0. I've implemented both of the suggestions that were given my question from last night, but now I'm even more at a loss that before. I'm not quite sure what this error means even, let alone why I am getting it. Below is my source code:

#include <stdio.h>
#include <stdlib.h>

struct node{
    char ANSII;
    struct node *next_node;
};

void clear_list(struct node *first_node);
void print(struct node *first_node);
int counter(struct node *first_node);
void append(char temp, struct node *first_node);


int main(int argc, char **argv){
    FILE *f = NULL;
    struct node header;
    char temp;

    if(argc != 2){ /* argv[0] = name of the program, argv[1] = file to open */
        printf("usage: %s filename:", argv[0]);
    }

    f = fopen(argv[1], "rb");

    if(f == 0){ /* check for successful read */
         printf("Could not open file.\n");
    }

    while(!feof(f)){
        fread(&temp, sizeof(1), 1, f);
        if(temp >= 32 && temp <= 128){ /* If it falls between the bounds of printable     characters. */
            append(temp, &header); //Builds the string
        }else{
            if(counter(&header) > 3){
                print(&header);
            }
            clear_list(&header);
        }
    }
    return 0;
}
void clear_list(struct node *first_node){
    struct node *conductor;
    while(first_node != NULL){
        conductor = first_node;
        while(conductor->next_node != NULL){
            conductor = conductor->next_node;
        }
        free(conductor);
    }
}
void print(struct node *first_node){
    struct node *conductor = first_node;
    while(conductor != 0){
        printf("%s", conductor->ANSII);
        conductor = conductor->next_node;
    }
    printf("\n");
}
int counter(struct node *first_node){
    struct node *conductor = first_node;
    int counter = 0;
    while(conductor != 0){
        conductor = conductor->next_node;
        counter++;
    }
    return counter;
}
void append(char temp, struct node *first_node){
    struct node *conductor = first_node;
    while(conductor->next_node != 0){
        conductor = conductor->next_node;
    }
    conductor->next_node = malloc(sizeof(conductor->next_node));
    if(conductor->next_node == 0){
        printf("Memory allocation failed!");
        return;
    }
    conductor = conductor->next_node;
    conductor->ANSII = temp;
}

I tried implementing the answers so far, and now instead of a segmentation fault I'm getting the following run-time crash:

*** glibc detected *** ./mystrings: double free or corruption (fasttop): 0x0000000000601250   ***
======= Backtrace: =========
/lib64/libc.so.6[0x3886a75916]
./mystrings[0x400798]
./mystrings[0x40072f]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x3886a1ecdd]
./mystrings[0x4005b9]
======= Memory map: ========
00400000-00401000 r-xp 00000000 00:1b 1921384528                         /afs/pitt.edu/home/n/a/nap54/private/cs449/project2/mystrings
00600000-00601000 rw-p 00000000 00:1b 1921384528                              /afs/pitt.edu/home/n/a/nap54/private/cs449/project2/mystrings
00601000-00622000 rw-p 00000000 00:00 0                                  [heap]
3886600000-3886620000 r-xp 00000000 fd:00 180                            /lib64/ld-2.12.so
388681f000-3886820000 r--p 0001f000 fd:00 180                            /lib64/ld-2.12.so
3886820000-3886821000 rw-p 00020000 fd:00 180                            /lib64/ld-2.12.so
3886821000-3886822000 rw-p 00000000 00:00 0
3886a00000-3886b89000 r-xp 00000000 fd:00 183                            /lib64/libc-2.12.so
3886b89000-3886d89000 ---p 00189000 fd:00 183                            /lib64/libc-2.12.so
3886d89000-3886d8d000 r--p 00189000 fd:00 183                            /lib64/libc-2.12.so
3886d8d000-3886d8e000 rw-p 0018d000 fd:00 183                            /lib64/libc-   2.12.so
3886d8e000-3886d93000 rw-p 00000000 00:00 0
388d200000-388d216000 r-xp 00000000 fd:00 6639                           /lib64/libgcc_s-4.4.6-20120305.so.1
388d216000-388d415000 ---p 00016000 fd:00 6639                           /lib64/libgcc_s-  4.4.6-20120305.so.1
388d415000-388d416000 rw-p 00015000 fd:00 6639                           /lib64/libgcc_s-    4.4.6-20120305.so.1
7ffff7fd5000-7ffff7fd8000 rw-p 00000000 00:00 0
7ffff7ffb000-7ffff7ffe000 rw-p 00000000 00:00 0
7ffff7ffe000-7ffff7fff000 r-xp 00000000 00:00 0                          [vdso]
7ffffffea000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted (core dumped)

Now I'm completely lost. Could someone shed (more?) insight? Thanks for your help guys, I haven't gotten much (any sleep) last night and this project (along with the more important second portion of it) is due at midnight tonight... I understand that I'm just kind of pasting my code and saying 'have at it', but I'm really not sure where to begin, as this is my first time experiencing a crash such as this.

about `feof`, see http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong — , Oct 15 '13 at 16:49

score 2 · Answer 1 · answered Oct 15 '13 at 16:45

2

You really ought to learn how to use a debugger like gdb or other.

One obvious problem is that you do not initialize the next field in the header structure. So when you go to append, it starts going through it and who knows where it might end up (do not assume the fields are initialized to 0)

answered Oct 15 '13 at 16:45

Matthieu

16,103
10
59
86

Thank you for your help. I forgot that I had updated my code from last night (where I drew the code from) to initialize the next field. I appreciate the help though! – Fiki Oct 15 '13 at 17:48

simonc · Accepted Answer · 2013-10-15T17:35:08.040

2

Your clear_list function frees the tail of the list then repeats. Once it reaches the point where there is only a single node remaining, it repeatedly tries to free first_node. clear_list could be more simply written as

void clear_list(struct node *first_node){
    struct node *ptr = first_node;
    struct node *next;
    while (ptr != NULL) {
        next = ptr->next_node;
        free(ptr);
        ptr = next;
    }
}

Another issue, originally pointed out by Grijesh Chauhan, is that the line

printf("%s", conductor->ANSII);

is wrong. conductor->ANSII has type char so you should use the %c format specifier to print its value.

edited Oct 15 '13 at 17:35

answered Oct 15 '13 at 16:45

simonc

41,632
12
85
103

I didn't notice this problem, add print issue also(deleted my answer). my vote limit reached today so consider for both of your answer pending :) – Grijesh Chauhan Oct 15 '13 at 16:50
@GrijeshChauhan Were you suggesting that I add the `printf` format specifier bug you spotted to my answer? I've done this now. Please feel free to edit my answer if that isn't what you wanted. – simonc Oct 15 '13 at 17:36
2

@simcon Yes this is what I wanted you to add in your answer. This is another issue in `print(){..}` function. I think OP was unaware of this at time he posted question. But it is not for which he asked question so I deleted my answer. – Grijesh Chauhan Oct 15 '13 at 17:44
Thank you very much - this also makes sense, and that is a clever way to use two pointers to simplify and more efficiently clear the linked list! – Fiki Oct 15 '13 at 17:46

score 1 · Answer 3 · answered Oct 15 '13 at 16:58

You used sizeof twice and both of them are wrong.

sizeof(1) is the size of an int, because 1 is an int constant. Most likely to be 4, with the typical 8-bit bytes and 32-bit ints. So your fread is trying to read 4 bytes into a single char. That's bad.

In the other one, you've almost used the recommended safe sizeof idiom for malloc:

p = malloc(sizeof *p);

but you forgot the *. So you're allocating enough space to hold the pointer to the struct, not enough to hold the struct.

conductor->next_node = malloc(sizeof(*conductor->next_node));

Completely right - the 1 was just a typo/logical error that must of occurred out of carelessness. But I didn't even realize that I was doing the malloc incorrectly for the new struct, I appreciate the clarification and reasoning. It was really helpful to my understanding. — Fiki, Oct 15 '13 at 17:44

Run-time crash in C program mimicking 'strings' UNIX command

3 Answers3