Segmentation Fault (Malloc/Free in a Loop)

Question

I've been revisiting the C language and am having trouble freeing up memory after use in my program:

    int tvalue = 2;
    while (smult == 0) {
        int * tvaluearray = calloc(allnum, sizeof(int));    
        tvaluearray = frequencyArray(tvalue, allnum, tvaluearray);
        printf("tvalue = %d\n", tvalue);    
        //compare each index of the tvaluearray and the first array
        for (int j = 0; j < allnum; j++) {
//          printf("tvaluearray[%d]=%d >= firstarray[%d]=%d\n", j, tvaluearray[j], j, firstarray[j]);
            if (tvaluearray[j] < firstarray[j]) {
            //  printf("Found the false statement\n");
                break;
            }
            else if ( (j+1) == allnum ){
                smult = 1;
//              printf("Made it to else if! smult = %d\n", smult);
            }
        }
        free(tvaluearray);
        ++tvalue;
    }

The frequencyArray function is shown below:

int * frequencyArray (int target, int allnum, int targetarray[]) {
    int divisor = 2;

    for (int i = 0; i < allnum; i++)
        targetarray[i] = 0;
    //find the common factor frequency of the given number
    while (target > 1) {
        if (target % divisor == 0) {
            targetarray[divisor] += 1;
            target /= divisor;
        }
        else
            ++divisor;
    }


    return targetarray;
}

Having played around with this a bit, I've tried the following with different results:

1) removing the free of targetarray:

tvalue = 1306 --> segfault

2) including the free(targetarray):

tvalue = 29 free(): invalid next size (fast) Aborted (core dumped)

3) including free(targetarray) AND allocating 4*sizeof(int) for the tvaluearray calloc as opposed to just int:

tvalue = 31468 --> segfault

The third test had me changing the allocated space for the array with varying results before my program runs into the segmentation fault error. This has me thinking that there's an issue with the way I'm allocating space, but I think it just might be a little bit beyond my current understanding. Do any of y'all see where I may be going wrong?

tvaluearray = calloc(...) directly followed by tvaluearray = frequencyArray(...)... Is that last assignment really needed? Do frequencyArray really need to return the targetarray argument? — Some programmer dude
Also in frequencyArray, what happens if the while loop runs a little to far, making divisor being out of bounds? — Some programmer dude
I think that it's a problematic design to assign the return from frequencyArray() to the pointer tvaluearray. That pointer is tracking your allocated memory! It should only be read from, not written to. Look at it this way: if the function frequencyArray() changes the value of the pointer, you're looking at undefined behavior (memory leak, crash, etc.). If it doesn't change the value, there's no point in returning anything. — Tim Randall
Another minor point:. You've used the variable name allnum in frequencyArray() and in the calling code. The compiler will have no doubt which one is going to be referred to, but you might get confused. Specifically, note that if you change the value in the function, the value seen by the calling code won't change. — Tim Randall
The divisor being out of bounds was the source of the error! Good to know about the array though. So is it a general rule of thumb to never pass pointers through a function? — Okeefe Niemann

VillageTech VillageTech · Accepted Answer · 2019-12-24T00:46:41

In frequencyArray() function you have a loop:

while (target > 1) {
    if (target % divisor == 0) {
        targetarray[divisor] += 1;
        target /= divisor;
    }
    else
        ++divisor;
}

where divisor is used as index of your targetarray[]. I can't see here any formal bounds for maximum value of divisor - it seems that it can grow over maximum allowed value (equal allnum - 1), so the memory outside targetarray[] can be overwritten resulting in memory corruption/segmentation fault. You should check in each iteration that divisor < allnum .

Unfortunately, I don't know the context of your code, so can't propose any appropiate solution here. Probably it should look like:

while (target > 1) {
    if (target % divisor == 0) {

        // 'fuse' ;)
        if (divisor >= allnum) {
           // place here the code needed to solve the problem or break to exit from loop
        }
        // end of 'fuse'

        targetarray[divisor] += 1;
        target /= divisor;
    }
    else
        ++divisor;
}

Segmentation Fault (Malloc/Free in a Loop)

1 Answers