CODEWITHSUNDEEP
cfreedynamic-memory-allocation
yunusaydin
yunusaydin

Reputation: 367

Runtime error due to free() function

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

int *seperateDigits(int n)
{
    int *digits, numberOfDigits = 0, temp = n;

    while (temp) {
        temp /= 10;
        numberOfDigits++;
    }

    digits = (int *)malloc(sizeof(int) * (numberOfDigits + 1));

    digits[numberOfDigits] = -1;
    while (numberOfDigits >= 0) {
        digits[--numberOfDigits] = n % 10;
        n /= 10;
    }

    return digits;
}

int main(void) {
    int n, *na;

    scanf("%d", &n);
    na = seperateDigits(n);

    if (!na) {
        printf("Cannot allocate memory!\n");
        return 1;
    }

    while (*na != -1)
        printf("%d ", *na++);

    free(na);

    return 0;
}

I wrote a function to seperate a number into its digits. Everthing works fine when I don't deallocate the memory; but when I try to deallocate the memory ,which is dynamically allocated, with free() function, I get a runtime error. What am I doing wrong?

Upvotes: 1

Views: 2088

Answers (3)

user7491277
user7491277

Reputation: 76

In addition to the post increment of na you have another problem. To understand it well, you need to understand how free works internally.

You can make a note that free doesn't ask you how much memory the pointer na is pointing to, and thus it makes the right deallocation and knows exactly how much memory is that. The question is how free get to know that information. And the response is that, internally, when you call malloc calloc or realloc those functions allocate more memory than you requested and that is roughly the amount you allocated + the amount of memory needed for information about that memory, especially its size.

So for simplicity's sake, imagine that if you do Tab=malloc (3*sizeof (int)) to have an array Tab of 3 integers what malloc really does is allocate 4*sizeof (int) bytes of memory, i.e. an array Tab0 of 4 integers in the first sizeof (int) bytes, i.e. Tab0 [0] it stores the size of the memory you asked for i.e Tab0[0] =3*sizeof (int); then it returns a pointer that points to the next 3*sizeof (int) bytes i.e. return &Tab0[1];. When free (Tab); is called, it knows that the pointer you gave it refers to the second member of the array so it decrement it and read how much memory it has to to free it does: 

int* Tab0=Tab-1;
int memoryToFree=Tab0[0];.

I like to note that what I said is not exactly what happens, it is an over simplification just to understand why you have the run time error. Normally in order to use malloc, realloc, calloc and free you don't have to understand how they internally work all you need to know is that you should pass to free only pointers that were returned by malloc realloc or calloc.

The problem you did is that you accessed and changed memory you shouldn't touch a memory that is crucial for free to work correctly you did that here:

while (numberOfDigits >= 0) {
    digits[--numberOfDigits] = n % 10;
    n /= 10;
}

when numberOfDigits==0 digits[--numberOfDigits] = n % 10; is equivalent to numberOfDigits=-1; digits[-1] = n % 10;. The system doesn't recognize this as a segmentation error because you actually own that memory it was allocated by malloc and you altered the data that store crucial information that free need to work properly. so, you can correct it by doing this:

while (numberOfDigits > 0) {
    digits[--numberOfDigits] = n % 10;
    n /= 10;
}

Upvotes: 0

ajay
ajay

Reputation: 9680

The free function must be passed the pointer returned by malloc. However, the post-increment operator changes the value of na in the printf call in main -

printf("%d ", *na++);

Also, you should check the return value of malloc for NULL immediately after calling malloc. Don't cast the result of malloc. It's not useful and can cause problems if you forget to include the header stdlib.h which contains its prototype. Read this - Do I cast the result of malloc? I suggest the following changes -

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

int *seperateDigits(int n)
{
    int *digits, numberOfDigits = 0, temp = n;

    while (temp) {
        temp /= 10;
        numberOfDigits++;
    }

    // don't cast the result of malloc
    digits = malloc((numberOfDigits + 1) * sizeof(*digits));

    // check the result of malloc just after calling it
    if(digits == NULL) {
        printf("malloc failed to allocate memory\n");
        return NULL;
    }

    digits[numberOfDigits] = -1;
    while (numberOfDigits >= 0) {
        digits[--numberOfDigits] = n % 10;
        n /= 10;
    }

    return digits;
}

int main(void) {
    int n, *na;
    int i = 0;

    scanf("%d", &n);
    na = seperateDigits(n);

    if(na == NULL)
        return 1;

    while (na[i] != -1) {
        printf("%d ", na[i]);
        i++;
    }

    free(na);

    return 0;
}

Upvotes: 2

The Paramagnetic Croissant
The Paramagnetic Croissant

Reputation: 9343

you increment na, so you don't free what malloc returned (but the pointer numberOfDigits places past it). use an explicit integer index instead.

oh, and don't cast the return value of malloc() to int *. it's harmful.

Upvotes: 3

Related Questions