Why am I getting a segmentation fault?

Question

Hey guys I'm trying to write a program that takes in a plaintext file as it's argument and parses through it, adding all the numbers together and then print out the sum. The following is my code:

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

static int sumNumbers(char filename[])
{
    int sum = 0;
    FILE *file = fopen(filename, "r");
    char *str;

    while (fgets(str, sizeof BUFSIZ, file))
    {
        while (*str != '\0')
        {
            if (isdigit(*str))
            {
                sum += atoi(str);
                str++;
                while (isdigit(*str))
                    str++;
                continue;
            }
            str++;
        }
    }

    fclose(file);

    return sum;
}

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        fprintf(stderr, "Please enter the filename as the argument.\n");
        exit(EXIT_FAILURE);
    }
    else
    {
        printf("The sum of all the numbers in the file is : %d\n", sumNumbers(argv[1]));
        exit(EXIT_SUCCESS);
    }

    return 0;
}

And the text file I'm using is:

This a rather boring text file with some random numbers scattered throughout it.

Here is one: 87 and here is another: 3

and finally two last numbers: 12 19381. Done. Phew.

When I compile and try to run it, I get a segmentation fault.

Answer 1

Because you've not allocated space for your buffer.

Answer 2

You've not allocated space for the buffer.
The pointer str is just a dangling pointer. So your program effectively dumps the data read from the file into memory location which you don't own, leading to the segmentation fault.

You need:

char *str;
str = malloc(BUFSIZ); // this is missing..also free() the mem once done using it.

or just:

char str[BUFSIZ]; // but then you can't do str++, you'll have to use another 
                  // pointer say char *ptr = str; and use it in place of str.

EDIT:

There is another bug in:

while (fgets(str, sizeof BUFSIZ, file))

The 2nd argument should be BUFSIZ not sizeof BUFSIZ.

Why?

Because the 2nd argument is the maximum number of characters to be read into the buffer including the null-character. Since sizeof BUFSIZ is 4 you can read max upto 3 char into the buffer. That is reason why 19381 was being read as 193 and then 81<space>.

Answer 3

You haven't allocated any memory to populate str. fgets takes as its first argument a buffer, not an unassigned pointer.

Instead of char *str; you need to define a reasonably sized buffer, say, char str[BUFSIZ];

Answer 4

You have declared char* str, but you have not set aside memory for it just yet. You will need to malloc memory for it.

Many memory related errors such as this one can be easily found with valgrind. I'd highly recommend using it as a debugging tool.

Answer 5

char *str;

str has no memory allocated for it. Either use malloc() to allocate some memory for it, or declared it with a predefined size.

char str[MAX_SIZE];

Answer 6

A number of people have already addressed the problem you asked about, but I've got a question in return. What exactly do you think this accomplishes:

        if (isdigit(*str))
        {
            if (isdigit(*str))
            {
                sum += atoi(str);
                str++;
                while (isdigit(*str))
                    str++;
                continue;
            }
        }

What's supposed to be the point of two successive if statements with the exact same condition? (Note for the record: neither one has an else clause).

Answer 7

Your program has several bugs:

• It does not handle long lines correctly. When you read a buffer of some size it may happen that some number starts at the end of the buffer and continues at the beginning of the next buffer. For example, if you have a buffer of size 4, there might be the input The |numb|er 1|2345| is |larg|e., where the vertical lines indicate the buffer's contents. You would then count the 1 and the 2345 separately.
• It calls isdigit with a char as argument. As soon as you read any "large" character (greater than SCHAR_MAX) the behavior is undefined. Your program might crash or produce incorrect results or do whatever it wants to do. To fix this, you must first cast the value to an unsigned char, for example isdigit((unsigned char) *str). Or, as in my code, you can feed it the value from the fgetc function, which is guaranteed to be a valid argument for isdigit.
• You use a function that requires a buffer (fgets) but you fail to allocate the buffer. As others noted, the easiest way to get a buffer is to declare a local variable char buffer[BUFSIZ].
• You use the str variable for two purposes: To hold the address of the buffer (which should remain constant over the whole execution time) and the pointer for analyzing the text (which changes during the execution). Make these two variables. I would call them buffer and p (short for pointer).

Here is my code:

#include <ctype.h>
#include <stdio.h>

static int sumNumbers(const char *filename)
{
    int sum, num, c;
    FILE *f;

    if ((f = fopen(filename, "r")) == NULL) {
        /* TODO: insert error handling here. */
    }

    sum = 0;
    num = 0;
    while ((c = fgetc(f)) != EOF) {
        if (isdigit(c)) {
            num = 10 * num + (c - '0');
        } else if (num != 0) {
            sum += num;
            num = 0;
        }
    }

    if (fclose(f) != 0) {
        /* TODO: insert error handling here. */
    }

    return sum;
}

int main(int argc, char **argv) {
    int i;

    for (i = 1; i < argc; i++)
        printf("%d\t%s\n", sumNumbers(argv[i]), argv[i]);
    return 0;
}

Answer 8

Here is a function, that does your job:

static int sumNumbers(char* filename) {
    int sum = 0;
    FILE *file = fopen(filename, "r");
    char buf[BUFSIZ], *str;

    while (fgets(buf, BUFSIZ, file))
    {
            str=buf;
            while (*str)
            {
                    if (isdigit(*str))
                    {
                            sum += strtol(str, &str, 10);
                    }
                    str++;
            }
    }
    fclose(file);
    return sum;
}

This doesn't includes error handling, but works quite well. For your file, output will be

The sum of all the numbers in the file is : 19483