segmentation fault in sorting program C

Question

I am writing code to sort a txt file of int's and then display them when the user asks for a number at a certain index but every time I run my code I get a segmentation fault. What can I do to fix this?

void insert_sorted(long *sorted, int count, long value)
{
    int i = 0;

    sorted[1024] = value;
    if (count == 0) return;
    for (i = count; i >= 0; i--) {
        if (value < sorted[i - 1])
            sorted[i] = sorted[i - 1];
        else break;
    }
    sorted[i] = value;
}
int main(int argc, char *argv[])
{
    FILE *infile = NULL;
    int count = 0;
    long sorted[1024];
    long value;
    int i = 0;
    if (argc < 2) {
        fprintf(stderr, "Usage : %s <file_name>/n", argv[0]);
        return 1;
    }
    infile = fopen(argv[1], "r");
    if (NULL == infile) {
        perror("fopen");
        return -1;
    }
    /* while file not ends */
    while (!feof(infile)) {
        fscanf(infile, "%ld\n", &value); /* fetch value */
        insert_sorted(sorted, count, value); /* sort */
        ++count; /* increase number of sorted values */
    }
    /* display values */
    printf("Enter Index : ");
    int index;
    scanf("%d", &index);
    if (index == -1)
        fclose(infile);
    printf("%d ", sorted[index]);

    /* cleanup */
    if (infile) {
        fclose(infile);
        infile = NULL;
    }
    return 0;
}

Answer 1

The operation in sorted[1024] =value; will crash your program. The size of sorted array is only 1024 so that the bigest index is 1023.

One way to fix that is to change the size of sorted to 1025 in the main() function.

An other operation in the for loop that can crash the program : while i =0, access to sorted [ i - 1 ] will catch an exception.

Answer 2

The size of array sorted is 1024:

    long sorted[1024];

which means the valid index of array sorted range from 0 to 1023.

In insert_sorted() function, you are trying to access sorted array beyond its range:

    sorted[1024] = value;
           ^^^^

Accessing an element of array beyond its size is undefined behavior which includes program may give segmentation fault.

In fact, there is a logical error in insert_sorted() function. It is never storing the first value at appropriate location in the sorted array because of this count value check:

    if (count == 0) return;

For the first time count value will be 0 and the function return without storing value at appropriate location in sorted array. Also, you should put a check on count value and if it is greater than the array size, function should give appropriate error message. You can do:

#define ARR_SZ 1024

void insert_sorted(long *sorted, int count, long value) {
    int i = 0;

    if (count > ARR_SZ) {
        fprintf (stderr, "Array is exhausted");
        //may you want to change the return type of function from void to 
        //int and return a value in case of error.
        //Based on the return value, the caller of this function can take
        //appropriate action.
        return;
    }

    for (i = count; i > 0; i--) {
        if (value < sorted[i - 1])
            sorted[i] = sorted[i - 1];
        else 
            break;
    }
    sorted[i] = value;
}

int main(int argc, char *argv[])
{
    FILE *infile = NULL;
    int count = 0;
    long sorted[ARR_SZ];
    ......
    ......
    ......
}

When you are taking index input from user, make sure to add a check whether it is less than 0 or greater than or equal to count. If it is, you should not access sorted[index].

    scanf("%d", &index);

    if ((index >= count) || (index < 0)) {
         //print error message and do not access sorted[index]
    }

You don't need this:

    if (index == -1)
        fclose(infile);

because of this when user enters -1, the fclose is getting called twice for infile. Make sure once you have closed the FILE * of a opened file, don't call fclose again on it.

Answer 3

Segmentation fault appear when you try to write in an element array with a key that is bigger than his dimension. In this case you are writing on

sorted[1024] = value

that has as biggest key 1023 (long sorted[1024] means from 0 to 1023).

You should change

long sorted[1024];

with

long sorted[1025];

else you should change

sorted[1024] = value;

with

sorted[1023] = value;

I didn't read your code for telling you which is better.

Answer 4

What can I do to fix this?

You can run the program in a debugger.

You can either step through the code, one instruction at a time, inspecting the values of your variables as you go, or you can simply let the program run under the watchful eye of the debugger. If and when the program causes a seg-fault, the debugger will interrupt it at the exact point of failure, allowing you to see which code is responsible. It will even let you see the values of the variables involved, if there's any doubt about why the code did what it did.