The right pattern for sequence of allocations and operations which may fail

Question

I have a piece of code which looks almost like this (this is simplified just to show the structure):

int Ugly_Calculation(void)
{
    type1 *X1 = type1_new();
    if (!X1) return 0;
    type2 *X2 = type2_new(X1);
    if (!X2) return 0;
    type3 *X3 = type3_new(X1, X2);
    if (!X3) return 0;
    int result = PerformCalculation(X1, X2, X3);
    free(X3);
    free(X2);
    free(X1);
    return result;
}

It allocates some objects and uses them for some calculation and frees the objects. The typeX_new functions quite often return zero, so the memory is usually not freed correctly.

In my first iteration, I have rewritten the code in following way:

typedef struct
{
    type1 *X1;
    type2 *X2;
    type3 *X3;
} ugly_context;

void free_ctx(ugly_context *ctx)
{
    free(ctx->X1);
    free(ctx->X2);
    free(ctx->X3);
}

int Ugly_Calculation(void)
{
    ugly_context ctx = {NULL, NULL, NULL};

    ctx->X1 = type1_new();
    if (!ctx->X1)
    {
        free_ctx(&ctx);
        return 0;
    }
    ctx->X2 = type1_new(ctx->X1);
    if (!ctx->X2)
    {
        free_ctx(&ctx);
        return 0;
    }
    ctx->X3 = type1_new(ctx->X1, ctx->X2);
    if (!ctx->X3)
    {
        free_ctx(&ctx);
        return 0;
    }
    int result = PerformCalculation(X1, X2, X3);
    free_ctx(&ctx);
    return result; 
}

My first fix is safe, but it has lost the readability of the original code. My second idea is following:

type2 *type2_new_check(type1 *X1)
{
   type2 *result = NULL;
   if (X1)
   {
       result = type2_new(X1);
   }
   return result;
}

type3 *type3_new_check(type1 *X1, type2 *X2)
{
   type3 *result = NULL;
   if (X1 && X2)
   {
       result = type3_new(X1, X2);
   }
   return result;
}

int PerformCalculation_check(type1 *X1, type2 *X2, type3 *X3)
{
    int result = 0;
    if (X1 && X2 && X3)
    {
       result = PerformCalculation(X1, X2, X3);
    }
    return result;
}

int Ugly_Calculation(void)
{
    type1 *X1 = type1_new();
    type2 *X2 = type2_new_check(X1);
    type3 *X3 = type3_new_check(X1, X2);
    int result = PerformCalculation_check(X1, X2, X3);
    free(X3);
    free(X2);
    free(X1);
    return result;
}

The code readability is OK and early exits are gone, but it is pretty long for method with 3 memory blocks.

My real code is not 3 allocations, but 15, so the method becomes very creepy and ugly. Is there any nice way, how to solve this pattern without excessive checks and freeing? (The best would be to break the method to smaller parts that are cleanly manageable by checks, but I'd like to avoid that for some reason)

score 3 · Answer 1 · answered Aug 16 '17 at 14:09

3

That's the (IMHO) one good use for goto (and a common C idiom):

    type *obj1 = 0;
    type *obj2 = 0;
    type *obj3 = 0;

    if (!(obj1 = create1())) goto error;
    if (!(obj2 = create2())) goto error;
    if (!(obj3 = create3())) goto error;

    // your logic here

    return 0; // success

error:
    free(obj3);
    free(obj2);
    free(obj1);
    return -1; // failure

Or, if you need to free the allocated resources in success case as well, like this:

    type *obj1 = 0;
    type *obj2 = 0;
    type *obj3 = 0;
    int success = -1;

    if (!(obj1 = create1())) goto error;
    if (!(obj2 = create2())) goto error;
    if (!(obj3 = create3())) goto error;

    success = 0; // success

    // your logic here

error:
    free(obj3);
    free(obj2);
    free(obj1);
    return success;

answered Aug 16 '17 at 14:09

This will not keep most compilers happy though, let along static analysers. They tend to dislike assignment inside conditions and `goto` both. Although this code is ok, I believe there are better ways. – Lundin Aug 16 '17 at 14:23
a paranthesized assignment is fine for the compilers I know, and `goto` of course is, as well. – Aug 16 '17 at 14:24
None of it will slip past a MISRA-C checker however. – Lundin Aug 16 '17 at 14:29
Fortunately, I can ignore their nonsense :) (Don't get me wrong on this, there are good intentions behind it, but it's **always** a problem to give some definitive **always** / **never** rules) – Aug 16 '17 at 14:30
@Lundin btw, because of such rules, people come up with ["creatively disguised gotos"](https://stackoverflow.com/a/45249553/2371524). Awesome readability ;) – Aug 16 '17 at 14:35

score 1 · Answer 2 · answered Aug 16 '17 at 14:21

1

The best way to handle this is often to use a couple of internal wrapper functions. Keep the allocation separate from the algorithm. Initialize the variables to 0 (NULL) and take advantage of free(NULL) being a well-defined no-op, meaning that everything will get cleaned up no matter which function that failed.

static int allocate (type1** X1, type2** X2, type3** X3)
{
  *X1 = type1_new(); if(*X1 == NULL) return 0;
  *X2 = type2_new(); if(*X2 == NULL) return 0;
  *X3 = type3_new(); if(*X3 == NULL) return 0;
  return 1;
}

int Ugly_Calculation (void)
{
  type1* X1 = 0;
  type2* X2 = 0;
  type3* X3 = 0;

  int result = allocate(&X1, &X2, &X3);
  if(result != 0)
  {
    result = PerformCalculation(X1, X2, X3);
  }

  free(X1);
  free(X2);
  free(X3);

  return result;
}

answered Aug 16 '17 at 14:21

Lundin

195,001
40
254
396

Another huge advantage of this is that you don't have to endure the `goto` debate yet again... – Lundin Aug 16 '17 at 14:21
I don't think there's much of a debate for using `goto` in exactly this scenario. But if you want to avoid it no matter what, that's probably the simplest way around. – Aug 16 '17 at 14:22
@FelixPalmen Give them a couple of hours and the debate will start. Did you not hear the news? [GoTo considered harmful](http://homepages.cwi.nl/~storm/teaching/reader/Dijkstra68.pdf)! :) – Lundin Aug 16 '17 at 14:26
of course, **breaking** news after 50 years ;) (but to be serious, I feel that almost the entire C community agrees that this scenario is a *positive* example for `goto`. Still nice to see a simple way to avoid it) – Aug 16 '17 at 14:26

The right pattern for sequence of allocations and operations which may fail

2 Answers2