Another way to do cleanup in C?

Question

Consider this program:

int main(void)
{
    int* i = malloc(sizeof(int));
    int* j = malloc(sizeof(int));
}

However this is a naive approach, because malloc may fail and the pointers are not free'd.

So you can do this:

int main(void)
{
    int* i; 
    int* j;

    if ((i = malloc(sizeof(int)) < 0)
    {
        return -1;
    }
    if ((j = malloc(sizeof(int)) < 0)
    {
        free(i);
        return -1;
    }

    free(i);
    free(j);
}

However I consider this very error-prone. Consider having to assign 20 pointers, in the last malloc error case, you have to free 19 variables and then return -1.

I also know atexit, which can help me to write it like this:

int* i; 
int* j;

void del_i(void)
{
    free(i);
}

void del_j(void)
{
    free(j);
}

int main(void)
{
    if ((i = malloc(sizeof(int)) < 0)
    {
        return -1;
    }
    else
    {
        atexit(del_i);
    }

    if ((j = malloc(sizeof(int)) < 0)
    {
        return -1;
    }
    else
    {
        atexit(del_j);
    }
}

Which is better, but I dislike having to declare all pointers as global. Is there some way to combine these two approaches, basically:

Having destructors for pointers, which can be either executed directly or be used with atexit.
Having pointers local to functions.

I don't get your point. If you're finishing/stopping the execution of the program, why bother about `free()`? — Sourav Ghosh, Mar 24 '16 at 15:46
1. You should always `free` pointers. 2. Consider this in the context of a larger program — hgiesel, Mar 24 '16 at 15:49
Thanks for enlightening me, but i'll stick to my version. :) — Sourav Ghosh, Mar 24 '16 at 15:50
Negative return values of a program are typically reserved for the system. An application should use small positive values and `0`. — too honest for this site, Mar 24 '16 at 16:00
malloc will return either `NULL` or a valid pointer. Checking for *"less than 0"* ( `< 0`) make no sense at all. — abelenky, Mar 24 '16 at 16:02
One thing to be aware of is that there may be a limited number of `atexit` slots (but at least 32). — Ian Abbott, Mar 24 '16 at 16:36
"Having destructors for pointers" - if you want C++ features, why not use C++? — sashoalm, Mar 24 '16 at 19:26

score 4 · Answer 1 · answered Mar 24 '16 at 16:02

4

free on NULL is defined to be a safe no-op. So a non-jumping variation could be:

int *i = malloc(sizeof(int)); 
int *j = malloc(sizeof(int));

if(i && j)
{
    // do some work
}

free(i);
free(j);

answered Mar 24 '16 at 16:02

Tommy

99,986
12
185
204

Sergey Kalinichenko · Answer 2 · 2016-03-24T16:02:02.763

First, this will not detect malloc failure:

if ((i = malloc(sizeof(int)) < 0)
{
    return -1;
}

malloc returns NULL on failure, not a negative number.

Second, atexit is good for cleaning up static and global objects. It is not a good idea to make local objects global only to use them inside atexit.

A better approach is to make a struct for all related pointers that you need to allocate in an all-or-nothing unit, define a function for freeing them all at once, and write a function that allocates them one by one with memory checking on each allocation:

typedef struct AllOrNothing {
    double *dPtr;
    int *iPtr;
    float *fPtr;
    size_t n;
} AllOrNothing;

void freeAllOrNothing(AllOrNothing *ptr) {
    free(ptr->dPtr);
    free(ptr->iPtr);
    free(ptr->fPtr);
    free(ptr);
}

int allocateAllOrNothing(size_t n, AllOrNothing **res) {
    *res = malloc(sizeof(AllOrNothing));
    if (*res == NULL) {
        return -1;
    }
    // Freeing NULL is allowed by the standard.
    // Set all pointers to NULL upfront, so we can free them
    // regardless of the stage at which the allocation fails
    (*res)->dPtr = NULL;
    (*res)->iPtr = NULL;
    (*res)->fPtr = NULL;
    (*res)->n = n;
    (*res)->dPtr = malloc(n*sizeof(double));
    if ((*res)->dPtr == NULL) {
        free(*res);
        *res = NULL;
        return -1;
    }
    (*res)->fPtr = malloc(n*sizeof(float));
    if ((*res)->fPtr == NULL) {
        free(*res);
        *res = NULL;
        return -1;
    }
    (*res)->iPtr = malloc(n*sizeof(int));
    if ((*res)->iPtr == NULL) {
        free(*res);
        *res = NULL;
        return -1;
    }
    return 0;
}

Schafwolle · Answer 3 · 2016-03-24T16:03:22.917

0

int main(void)
{
int* i = NULL; // Init with NULL otherwise free on none NULL possible
int* j = NULLL;

if (!(i = malloc(sizeof(int)))
{
    goto exit;
}
if (!(j = malloc(sizeof(int)))
{
    goto exit;
}
...
exit:
    free(i);
    free(j);
    ...
    return err;
}

This is something you can solve with goto statements.

edited Mar 24 '16 at 16:03

answered Mar 24 '16 at 15:53

Schafwolle

501
3
15

2

You jump to `exit` when the allocations succeed. Not so helpful. – unwind Mar 24 '16 at 16:00
1

Just be wary of http://www.dwheeler.com/essays/apple-goto-fail.html -type mistakes. – Tommy Mar 24 '16 at 16:04

score 0 · Answer 4 · answered Mar 24 '16 at 16:06

int main(void)
{
    int* i = NULL; 
    int* j = NULL;
    bool success = false;

    do {
        i = malloc(sizeof(int));
        if (NULL == i) break;

        j = malloc(sizeof(int));
        if (NULL == j) break;

        success = true;
    } while (0);

    if (!success)
    {
        printf("Something failed!");
    }
    else
    {
        printf("All succeeded!");
        // Do more work
    }

    free(i);
    free(j);
    return (success? 0 : 1);
}

score 0 · Answer 5 · answered Mar 24 '16 at 19:20

Avoid multiple exit points. Avoid interlacing allocation and error handling. Follows a clean order of operation:

Declare, allocate and initialize resources..
If all successful, do the task.
Clean-up.
Return status.

 // Do all allocations first, test their `NULL`-ness, then free them all.
 int main(void) {
    // Allocate resources   
    // declare and allocate in one step
    int* i    = malloc(sizeof *i);
    double* j = malloc(sizeof *j);

    // Test for acceptability
    bool ok = i && j;

    // Perform the main body of code
    if (ok) {
      ; // do normal process in the code;
    }

    // free resources
    free(i);
    free(j);

    // return status
    return ok ? 0 : -1;
}

score -1 · Answer 6 · answered Mar 24 '16 at 16:30

    int *i=NULL,*j=NULL;

    if(!(i=malloc(sizeof(int))))
            goto EXIT;
    if(!(j=malloc(sizeof(int))))
            goto EXIT;
    /* do some work */
    return 0;
    EXIT:
            free(i);
            free(j);
            exit(EXIT_FAILURE);

Although goto is considered a bad programming practice but here we can use it to get our task done with ease and simplicity

Another way to do cleanup in C?

6 Answers6