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?

Greatest

#5677847 – Verboten

One of the strongest reasons I can think of to always free resources, even if it's at exit time, is that it keeps the noise away when analyzing a program with Valgrind and/or LeakSanitizer in search for real memory leaks. – Deprive

Probably that and I guess also to teach us how to write code when you don't have an OS doing the clean up for you? The class is called "operating systems" – Greatest

use atexit and in that function perform the cleanups. – Gelation

@Greatest free(a); if(some_sys_call() == -1){ free(a); looks like a double free error. Was that your intent? – Haff

No, it was a mistake. Fixed it, thanks for pointing that out. – Greatest

'free all resources at program's termination' - that is not guaranteed to be possible with user code, never mind the morass of issues thst will arise with more complex apps:( – Gelatinous

'The class is called "operating systems"' aha! I suspect that the exercise is designed to demonstrate that such an effort is unsustainable during development. Without OS support, effectively terminating a process is impo....'very, very difficult':) – Gelatinous

I mean, just consider this: every time you add more code/functionality, you will have to go through a retest of your user cleanup code to make sure it still works:(( – Gelatinous

...and with more complex apps, you will have no idea, on an instantaneous basis, which thread currently owns what buffer pointer... – Gelatinous

In summary, I guarantee that attempting to clean up every resource with user code will result in a geometric explosion of effort, bugs and frustration with app complexity:( – Gelatinous

Just call your OS 'terminateProcess()' call, you know it makes sense:) – Gelatinous

A real world example was AmigaOS. There was no resource tracking, so a crashed process would leave memory allocated. The main reason was that all memory was shared between processes, so ownership of a shared memory segment was unclear. Not secure, but it worked most of the time for single users. There were utilities to try to clean things up, but generally rebooting was just accepted as something you do regularly (e.g. instead of sleep mode - there was no "shutdown", you just hit the power switch, and AmigaOS could boot about as fast as Linux waking and entering a password). – Muoimuon

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;

Dysgenic

This is one of the few acceptable uses of goto in C code. – Flashbulb

Using a macro in place of the goto here would be bad? Something like my free_all – Greatest

@Greatest Using a macro is often bad. Imagine you add char *d, you would have to update a macro and all macro usages. Whereas you would add above a single line with free(d). A macro is always defined globally, you have to use #undef if you need specific free_all in different functions. – Dysgenic

@273K If you were to add some variable, wouldn't you just need to change the macro and not the macro usages? Also to address your second point, you could define multiple free macros like: free_main, free_func1, etc. Is using macros considered bad in general? I usually use them almost as void functions that share scope with the caller. – Greatest

Never thought I'd see the day where a goto solution is not only lauded but gets top billing. For newbs, some history: goto has been a valid keyword from the beginning, but for reasons, coders came to consider it a tool only of the slothful, and posting it would get you excoriated in a hurry. This recognition of the value of the humble goto is at once overdue and stunning, lol – Aurel

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;
}

Haff

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);

Aurel

Agree. And: if you free some resource which is no longer needed -- assign NULL to the pointer just after that, to be sur it is already deallocated. – Alfonzoalford

You don't need to check if(a) because free(NULL) is a documented no-op. C18 7.22.3.3 says If ptr is a null pointer, no action occurs. – Licorice

greg spears, if(a) free(a); simpler as free(a);. – Haff

All -- thanks for the illumination that pointers no longer need to be checked before calling free(). Back in the day, I crashed a few apps by trying to free() stuff that wasn't allocated, and now I've built this habit of checking which I can gratefully drop! – Aurel

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.

Melba

This seems to be the most versatile and "high level" solution, unless I'm missing something. – Greatest

@Ernaldo, What is missing is that the question was more general than a linked-list. – Haff

@chux-ReinstateMonica The other solutions don't allow you for example to declare pointers to allocated memory inside nested blocks. I think you also need to declare all those pointers specifically at the start of the code for your solution to work. The hash map solution doesn't have these limitations I think. Anyway I noticed my main problem was that my functions were getting too large, if I make them smaller the "goto" solution would probably work best. – Greatest

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).

Muoimuon