What is a good coding practice for freeing allocated resources at failure/exit points in C?

Question

I'm working on a school project and the teacher asked us to free all resources at program's termination.

I'm struggling to find a way to write more readable and/or less code to manage that, specifically considering this is made more complex by the fact that different exit points will probably have a different set of resources to free.

The simplest and most messy solution seems to be this(exit points are represented as "some_sys_call" calls that return -1):

char *a = malloc(1);
if(some_sys_call() == -1){
   free(a);
   return -1;
}
//b is needed until the end
char *b = malloc(1);
if(some_sys_call() == -1){
   free(a);
   free(b);
   return -1;
}
//c is needed until the end
char *c = malloc(1);
//a is no longer needed from here on, so it's freed right away
free(a);
if(some_sys_call() == -1){
   free(b);
   free(c);
   return -1;
}
//Exit point when there are no errors
free(b);
free(c);
return 0;

This doesn't seem very appealing for obvious reasons: you need to write a lot of code especially when you have a lot of resources, making the code bloated with frees and less readable. Of course you can't simply write a macro or function that frees all the resources and call it at each exit point, like this:

#define free_all  free(a); \
                  free(b); \
                  free(c);

You could technically define different macros/functions for each set of frees:

#define free_1 free(a); 

#define free_2 free(a); \
               free(b);
...

This would make the "actual" code more readable but would still require you to write a lot of different macros and doesn't seem to me like a good solution either.

Is this considered a common problem? How is this usually handled?

Answer 1

This is a well known practice in C, although if it is best or not is disputable

char *a = malloc(1);
char *b = NULL;
char *c = NULL;
int ret = 0;

if(some_sys_call() == -1) {
  goto fail;
}

b = malloc(1);
if(some_sys_call() == -1) {
  goto fail;
}

c = malloc(1);
if(some_sys_call() == -1) {
  goto fail;
}

goto cleanup;

fail:
  ret = -1;
cleanup:
  free(c);
  free(b);
  free(a);

return ret;

The same but a bit shorter (inspired by proxyres /resolver_win8.c:338):

char *a = malloc(1);
char *b = NULL;
char *c = NULL;
int ret = 0;

if(some_sys_call() == -1) {
  goto fail;
}

b = malloc(1);
if(some_sys_call() == -1) {
  goto fail;
}

c = malloc(1);
if(some_sys_call() == -1) {
fail:
  ret = -1;
}

free(c);
free(b);
free(a);

return ret;

Answer 2

Variation on the @273K good answer.

// Return 0 on success
int foo() {
  // Initialize all with "failed" default values
  char *a = NULL;
  char *b = NULL;
  char *c = NULL;
  int ret = -1;

  a = malloc(1);
  if(a == NULL || some_sys_call() == -1) {
    goto cleanup;
  }

  b = malloc(1);
  if(b == NULL || some_sys_call() == -1) {
    goto cleanup;
  }

  c = malloc(1);
  if(c == NULL || some_sys_call() == -1) {
    goto cleanup;
  }

  // Assign success only if we made it this far.
  ret = 0; // Success!

  cleanup:
  free(c);
  free(b);
  free(a);
  return ret;
}

Answer 3

There are a few ways to do, but one is to have a function like so:

void free_all(void *a, void *b, void *c )
{
    /* We check to see if the pointer is actually allocated.  You should declare
    ** each pointer in the main module like so:  char *a = NULL, *b = NULL; */
    if(a)
        free(a);
    /*... and repeat for each and every parameter pointer... */
}

Then you can implement in main() like so:

if(some_error)
    free_all(a,b,c);

Answer 4

While this veers heavily into opinion territory, you might implement a linked list data structure that holds pointers to allocated memory and add them as they are successfully allocated. Then at a failure, you need only walk that list and free each pointer.

If you use a map, you can remove pointers from it if they are freed in the normal course of the program's execution.

Answer 5

My general strategy is to separate allocate/free from usage, like this:

int f(int x) {
    X_Buf *x_buf = malloc(x * ...);
    int f_ret = f_with_buf(x, x_buf);
    free(x_buf);

    return f_ret;
}

This also works with other resources, like open/close, fopen/fclose, mmap, and so on. The f_with_buf() function can return normally, or early with errors, and never worry about the need to free, since it didn't do the malloc.

You can chain these together when you need to allocate more than one thing, and there are dependencies:

int f(int x, double y) {
    X_Buf *x_buf = malloc(x * ...);
    int f_ret = f_with_x_buf(x, x_buf, y);
    free(x_buf);

    return f_ret;
}
int f_with_x_buf(x, x_buf, y) {
    int f_ret = f_x(x, x_buf);
    if (OK != f_ret) return f_ret;

    Y_Buf *y_buf = malloc(...);
    y_buf.x_buf = x_buf;

    f_ret = f_y(y, y_buf);
    free(y_buf);

    return f_ret;
}

But not all code has a nice obtain-use-release pattern. In those cases I think the best you can do is put a reference to any resources that need to be cleaned up in a global location, then use a cleanup function, either called explicitly, or using atexit(). Exactly what to keep and how to reference it depends on the app.

You can also kind of follow C++ practice. Although you can't have constructors and destructors, you can have functions that allocate and acquire all resources a struct needs, and another that deallocates them and frees it, e.g struct Y *y_new() and y_del(struct Y *y).