How to handle repetitive mallocs/frees in a DRY way in C

Question

So I have a function which is like a constructor for my structure:

MyStructure* CreateMyStructure(...)
{
    MyStructure *my_structure;
    double *data;
    int *colind, *rowptrs;

    data = malloc(sizeof(double)*N);
    if (data == NULL) { return(NULL); }

    colind = malloc(sizeof(int)*N);
    if (colind == NULL) { free(data); return(NULL); }

    rowptrs = malloc(sizeof(int)*K);
    if (rowptrs == NULL) { free(colind); free(data); return(NULL); }

    my_structure = malloc(sizeof(my_structure));
    if (my_structure == NULL) { 
        free(rowptrs); free(colind); free(data);
        return(NULL);
    }

    return(my_structure);
} 

This pattern is reoccurring in a lot of the codebase, so I am trying to come up with something cleaner. I thought about creating some macros like these:

#define CHECK_MALLOC1(var, dep1) \
    do { \
        if (var == NULL) { free(dep1); return(NULL); } \
    } while(0)

#define CHECK_MALLOC2(var, dep1, dep2) /* pattern continues */

However, I decided against them because I do not want to change the control flow inside a macro. Is there an alternative pattern that would help reduce repetition without sacrificing someones ability to understand what is going on?

Answer 1

Because C doesn't have RAII, using goto for error-handling (only jumping forwards to the error-handler, never back) is customary, especially in high-performance high-reliability code like kernels.

When using state, additional functions, or convoluted stucture instead, the danger is that the compiler might not reliably inline and de-convolute everything.

Your code re-written:

MyStructure* CreateMyStructure(...)
{
    MyStructure *my_structure;
    double *data;
    int *colind, *rowptrs;

    data = malloc(N * sizeof *data);
    if (!data) goto exit_0;

    colind = malloc(N * sizeof *colind);
    if (!colind) goto exit_1;

    rowptrs = malloc(K * sizeof *rowptrs);
    if (!rowptrs) goto exit_2;

    my_structure = malloc(sizeof *my_structure);
    if (!my_structure) goto exit_3;

    // More work here ...

    return my_structure;

exit_3:
    free(rowptrs);
exit_2:
    free(colind);
exit_1:
    free(data);
exit_0:
    return NULL;
}

As an aside, refrain from using sizeof (TYPE): Getting the type wrong initially, or not updating it everywhere in lock-step is a real danger. sizeof expr doesn't suffer from that.

Also, return is not a function. Beware of doing that especially if you also write C++.

Answer 2

The pattern to use here is goto CLEANUP. Some people freak out over gotos but they are the simplest approach in C. Since you have to clean up resources, having a single return and doing all cleanup just before that return will usually work best.

In this case cleanup only seems to be needed on error, not on success. So we won't put the cleanup code before the main return but add an error section. Your code would then become:

MyStructure* CreateMyStructure(int N, int K)
{
    MyStructure *my_structure = NULL;
    double *data = NULL;
    int *colind = NULL
    int *rowptrs = NULL;

    data = malloc(sizeof(double) * N);
    if (!data) goto ERROR;

    colind = malloc(sizeof(int) * N);
    if (!colind) goto ERROR;

    rowptrs = malloc(sizeof(int) * K);
    if (!rowptrs) goto ERROR;

    my_structure = malloc(sizeof(my_structure));
    if !(my_structure) goto ERROR;

    /* .. set up my_structure in a way that takes
     * ownership of data, colind, rowptrs */

    return my_structure;

ERROR:
    free(rowptrs);
    free(colind);
    free(data);
    return NULL;
}

Note that free() does nothing if the pointer is null, so we can manage with a single error/cleanup section. If you have to perform more complicated cleanup you will either have to guard those sections with conditionals, or have multiple labels for different stages of cleanup.

If keeping track of all allocated resources is too complicated, consider using C++ instead. Rather than using a raw malloc() or new, you can use smart pointers to automatically free the memory when you return or throw an exception. That makes resource leaks less likely and you more productive. By protecting the necessary extern "C" declarations with ifdefs, you can write headers that are compiled equivalently for C and C++, and can therefore use C++ selectively for some compilation units without having to immediately convert the whole project.

Answer 3

FYI, beyond amons excellent answer, there also other possible solutions to consider based on your other requirements.

---

You might package the sizes using an array, and use an array-oriented allocator:

int sizes [] = { sizeof(double)*N, sizeof(int)*N, sizeof(int)*K, sizeof(my_structure) };
void *allocations[ sizeof(sizes)/sizeof(int) ];
multi_alloc ( allocations, sizes, sizeof(sizes)/sizeof(int) );
if ( allocations[0] == NULL ) return NULL;

multi_alloc would allocate each item in turn, and cleanup as needed.  (You will still need to initialize my_structure that is returned, but that initialization is missing in your example as well.)

---

If you know that the lifetime of your sub-entities is all the same, you can allocate them all at once, using:

my_structure = malloc ( sizeof(double)*N + sizeof(int)*N + sizeof(int)*K + sizeof(my_structure) );

Now we need do nothing on failure (since there is only one allocation), and, you only need free ( my_structure ); to release it.  You will still need to initialize my_structure, in this case, using pointer arithmetic:

my_structure.doubleArray = (void*)my_structure + sizeof(my_structure);
my_structure.intArray = (void*)my_structure.doubleArray + sizeof(double) * N;
...

The main caveat with this approach is the issue of alignment: each individual malloc will ensure maximum necessary alignment, e.g. for doubles.  This does waste some space (as sometimes the malloced memory is used for int's not for double's), as well as other overhead of allocation of a memory block.  However, when doing a single malloc, we don't have those features (or their costs).

So, one general strategy to handle alignment, would be to sub-allocate out of the single malloc in the order of decreasing alignment requirements (so the double arrays first, then the int arrays).  If you want my_structure to be directly freeable (meaning it is the first item), you would need to make sure it has at least one double in it (even as a dummy union).

This would be appropriate as an optimization of both speed & space when millions of these objects are allocated.