How to avoid long chain of free's (or deletes)

Question

Suppose I write my code very defensively and always check the return types from all the functions that I call.

So I go like:

char* function() {
    char* mem = get_memory(100);  // first allocation
    if (!mem) return NULL;

    struct binder* b = get_binder('regular binder');  // second allocation
    if (!b) {
        free(mem);
        return NULL;
    }

    struct file* f = mk_file();  // third allocation
    if (!f) {
        free(mem);
        free_binder(b);
        return NULL;
    }

    // ...
}

Notice how quickly free() things get out of control. If some of the function fails, I have to free every single allocation before. The code quickly becomes ugly and all I do is copy paste everything over. I become a copy/paste programmer, even worse, if someone adds a statement somewhere in between, he has to modify all the code below to call the free() for his addition.

How do experienced C programmers go about this problem? I can't figure anything out.

Thanks, Boda Cydo.

Answer 1

I know I'll get lynched for this, but I had a friend who said they used goto for that.

Then he told me that it wasn't enough in most cases and he now used setjmp()/longjmp(). Basically he reinvented C++'s exceptions but with much less elegance.

That said, since goto could work, you could refactor it into something that doesn't use goto, but the indentation will get out of hand fast:

char* function() {
    char* result = NULL;
    char* mem = get_memory(100);
    if(mem) {
        struct binder* b = get_binder('regular binder');
        if(b) {
            struct file* f = mk_file();
            if (f) {
                // ...
            }
            free(b);
        }
        free(mem);
    }
    return result;
}

BTW, scattering the local variable declarations around the block like that isn't standard C.

Now, if you realize that free(NULL); is defined by the C standard as a do-nothing, you can simplify the nesting some:

char* function() {
    char* result = NULL;

    char* mem = get_memory(100);
    struct binder* b = get_binder('regular binder');
    struct file* f = mk_file();

    if (mem && b && f) {
        // ...
    }

    free(f);
    free(b);
    free(mem);

    return result;
}

Answer 2

If you want to do it without goto, here is an approach that scales well:

char *function(char *param)
{
    int status = 0;   // valid is 0, invalid is 1
    char *result = NULL;
    char *mem = NULL:
    struct binder* b = NULL;
    struct file* f = NULL:

    // check function parameter(s) for validity
    if (param == NULL)
    {
        status = 1;
    }

    if (status == 0)
    {
        mem = get_memory(100);  // first allocation

        if (!mem)
        {
            status = 1;
        }
    }

    if (status == 0)
    {
        b = get_binder('regular binder');  // second allocation

        if (!b)
        {
             status = 1;
        }
    }

    if (status == 0)
    {
        f = mk_file();  // third allocation

        if (!f)
        {
             status = 1;
        }
    }

    if (status == 0)
    {
        // do some useful work
        // assign value to result
    }

    // cleanup in reverse order
    free(f);
    free(b);
    free(mem);

    return result;
}

Answer 3

You could define a new label that would free the resources and then you could GOTO it whenever your code fails.

char* function()
{
    char* retval = NULL;
    char* mem = get_memory(100);  // first allocation
    if (!mem)
        goto out_error;

    struct binder* b = get_binder('regular binder');  // second allocation
    if (!b)
        goto out_error_mem;

    struct file* f = mk_file();  // third allocation
    if (!f)
        goto out_error_b;

    /* ... Normal code path ... */
    retval = good_value;

  out_error_b:
    free_binder(b);
  out_error_mem:
    free(mem);
  out_error:
    return retval;
}

Error management with GOTO was already discussed here: Valid use of goto for error management in C?

Answer 4

You could also take the opposite approach and check for success:

struct binder* b = get_binder('regular binder');  // second allocation
if(b) {
   struct ... *c = ...
   if(c) {
      ...
   }
   free(b);
}

Answer 5

If your data structures are complicated/nested, a single goto might not suffice, in which case I suggest something like:

mystruct = malloc(sizeof *mystruct);
if (!mystruct) goto fail1;
mystruct->data = malloc(100);
if (!mystruct->data) goto fail2;
foo = malloc(sizeof *foo);
if (!foo) goto fail2;
...
return mystruct;
fail2:
free(mystruct->data);
fail1:
free(mystruct);

A real world example would be more complicated and could involve multiple levels of structure nesting, linked lists, etc. Note here that free(mystruct->data); cannot be called (because dereferencing an element of mystruct is invalid) if the first malloc failed.

Answer 6

While I admire your approach to defensive coding and that's a good thing. And every C Programmer should have that mentality, it can apply to other languages as well...

I have to say this is the one thing useful about GOTO, despite purists will say otherwise, that would be an equivalent of a finally block but there's one particular gotcha that I can see there...

karlphillip's code is nearly complete but .... suppose the function was done like this

    char* function() {
      struct file* f = mk_file();  // third allocation
      if (!f) goto release_resources;
      // DO WHATEVER YOU HAVE TO DO.... 
      return some_ptr;
   release_resources:
      free(mem);
      free_binder(b);
      return NULL;
    }

Be careful!!! This would depend on the design and the purpose of the function that you see fit, that put aside.. if you were to return from the function like that, you could end up falling through the release_resources label.... which could induce a subtle bug, all the references to the pointers on the heap are gone and could end up returning garbage... so make sure if you have the memory allocated and return it back, use the return keyword immediately before the label otherwise the memory could disappear... or create a memory leak....