Recommended way to handle multiple malloc errors in a single function in C

Question

What is the recommended way to handle multiple malloc errors that might happen sequentially as in the following code?

bool myFunc(int x, int y)
{
    int *pBufX = null;
    int *pBufY = null;

    if((x <= 0) || (y <= 0))
    {
        return false;
    }

    pBufX = (int*)malloc(sizeof(int) * x);
    if(pBufX == null)
    {
        return false; 
    }

    pBufY = (int*)malloc(sizeof(int) * y);
    if(pBufY == null)
    {
        free(pBufX) //free the previously allocated pBufX 
        return false; 
    }

    //do something useful

    free(pBufX);
    free(pBufY);

    return true; 
}

The problem with this approach is that if the number of mallocs are high, you might forget to free some and cause memory leaks. Also, if there is some sort of log that needs outputting when an error occurs, the code becomes very long.

I have seen code that handles these with a goto, where you clear all of the mallocs in one place, only once. The code is not long, but I don't like using gotos.

Is there a better way than either of these two approaches?

Perhaps the problem is with the design in the first place. Is there a rule of thumb when designing functions when it comes to minimizing multiple mallocs?

Edit: There is another way that I have seen and used. Instead of using goto, you keep a status of the program and proceed only if the status is OK. Similar to goto but not using goto. But that increases the number of if statements which might make the code run slower.

bool myFunc(int x, int y)
{
    int *pBufX = null;
    int *pBufY = null;
    bool bRet  = true;

    if((x <= 0) || (y <= 0))
    {
        return false;
    }

    pBufX = (int*)malloc(sizeof(int) * x);
    if(pBufX == null)
    {
       bRet = false; 
    }

    if(bRet == true)
    {
        pBufY = (int*)malloc(sizeof(int) * y);
        if(pBufY == null)
        {
            bRet = false;
        } 
    }

    //do something useful

    if(pBufX != null)
        free(pBufX);

    if(pBufY != null)    
        free(pBufY);

    return bRet; 
}

That is one of the rare cases where `goto` can be really useful. — Jabberwocky, Dec 09 '15 at 06:47
Just off topic **-** Don't `free` `x` and `y` they are not pointers with allocated memory . — ameyCU, Dec 09 '15 at 06:49
Ignoring all variants of obfuscation such as your 2nd example, there's just 3 sensible ways of non-conditional branching that you can use here: return from function (ugly), goto fail (uglier) and loop+break (ugliest). I'd go for the least ugly. — Lundin, Dec 09 '15 at 07:31

Jabberwocky · Answer 1 · 2015-12-09T06:58:19.577

4

This is a possible solution:

bool myFunc(int x, int y)
{
    int returnvalue = false;

    int *pBufX = NULL;   // << null -> NULL
    int *pBufY = NULL;

    if((x <= 0) || (y <= 0))
    {
        goto fail;
    }

    pBufX = (int*)malloc(sizeof(int) * x);
    if(pBufX == null)
    {
        goto fail; 
    }

    pBufY = (int*)malloc(sizeof(int) * y);
    if(pBufY == null)
    {
        goto fail;
    }

    //do something useful

    returnvalue = true;    

fail:
   free(pBufX);   // <<< free(x) -> free(pBufX)
   free(pBufY);   // <<< free(y) -> free(pBufY)

   return returnvalue;
}

I would not recommed your second ("evil" goto avoiding) solution. It is more complicated that the goto solution.

BTW it is safe to free a null pointer, therefore

if(pBufY != NULL)     // NULL and not null
  free(pBufY);        // not y but pBufY

can be replaced by:

  free(pBufY);

edited Dec 09 '15 at 06:58

answered Dec 09 '15 at 06:51

Jabberwocky

48,281
17
65
115

I see. Forgot that you could free a null pointer with the generic free(). It is not allowed in the memory manager that our code is employing. – Anusha Dharmasena Dec 09 '15 at 06:59
While this solution is ok (even though it is a design pattern from 1980s BASIC programming :) ), I believe it is much cleaner just to return from the function. Ideally, you would split memory allocation from the actual algorithm, if possible, so that the caller handles all allocation – Lundin Dec 09 '15 at 07:22
@nushydude if `free(NUL)` is not allowed by your library (this is absolutely non standard BTW), then you can wrap this behaviour in a `myfree` function which ignores null pointers. – Jabberwocky Dec 09 '15 at 07:43
@Lundin Yep, I believe the function can be designed so this issue never shows up. Sadly that's for new projects. – Anusha Dharmasena Dec 10 '15 at 00:19

score 1 · Answer 2 · answered Dec 09 '15 at 06:50

1

Personally, I prefer using goto. But since it is safe to free(NULL), you can usually do all the allocations up front:

int *a = NULL; 
int *b = NULL;
a = malloc( sizeof *a * x);
b = malloc( sizeof *b * y);
if( a == NULL || b == NULL ) {
    free(a); 
    free(b); 
    return false;
}

If you need the allocated memory to do a computation to get a size for a further allocation, your function is probably sufficiently complex that it should be refactored anyway.

answered Dec 09 '15 at 06:50

William Pursell

204,365
48
270
300

If you're doing the allocations up front, you could use `int *a = malloc(sizeof(*a) * x); int *b = malloc(sizeof(*b) * y);` rather than explicitly setting to null and then assigning. The compiler probably does that anyway, so it is hardly crucial. I agree with the refactoring comment. – Jonathan Leffler Dec 09 '15 at 07:00
Sadly I am used to how the memory manager that we are using operates. It cannot free null pointers (gives an assert) so we have to check for null pointers before freeing, hence we always initialize them to null and go about doing our thing. I just wrote malloc and free for this example. And yes, the compiler ought to optimize them anyways. Just makes our code longer. – Anusha Dharmasena Dec 09 '15 at 07:08

score 1 · Answer 3 · edited May 23 '17 at 11:59

Great question! (I thought I would find a dupe of it, but no, weird, such an important aspect in C seemingly never really asked before)

There are two questions I found covering some of this field:

These mainly focus on the goto way. So first let's cover that.

The goto way

If you have a code which depends on allocating several resources which have to be released afterwards, you might use a pattern like below:

int function(void){
    res_type_1 *resource1;
    res_type_2 *resource2;

    resource1 = allocate_res_type_1();
    if (resource1 == NULL){ goto fail1; }
    resource2 = allocate_res_type_2();
    if (resource2 == NULL){ goto fail2; }

    /* Main logic, may have failure exits to fail3 */

    return SUCCESS;

    fail3:
    free_res_type_2(resource2);
    fail2:
    free_res_type_1(resource1);
    fail1:
    return FAIL;
}

You may read more on this approach on the excellent Regehr blog: http://blog.regehr.org/archives/894 also pointing to the Linux kernel itself which uses frequently this pattern.

Arrow code

This is one of the possible ways to do it otherwise. The above example looks like this with an "arrow" pattern:

int function(void){
    res_type_1 *resource1;
    res_type_2 *resource2;
    int        ret = FAIL;

    resource1 = allocate_res_type_1();
    if (resource1 != NULL){
        resource2 = allocate_res_type_2();
        if (resource2 != NULL){

            /* Main logic, should set ret = SUCCESS; if succeeds */

            free_res_type_2(resource2);
        }
        free_res_type_1(resource1);
    }

    return ret;
}

The obvious problem by which the pattern got its name is the potentially deep nesting (the code looking like an arrow), why this pattern is not liked that much.

Other ways

There is no much I can think of with the requirement of allocating and freeing resources (except for the flagging variant you detail in your question). You have more freedom when you have no such constraint, you can see some good approaches described in the other questions and answers.

If the resources are not dependent on each other, you might also use other patterns (such as early return which your first code example provides), however these two are those which can deal the right way with resource depencies if you need that (that is, resource2 can only be allocated on top of a valid resource1), and if the free function of the given resource doesn't handle a failed allocation's return.

Lundin · Answer 4 · 2015-12-09T07:41:21.210

The most proper way to do this in my opinion, is to move the core functionality to a separate function:

inline bool doStuff (int x, int y, int* pBufX, int* pBufy)

bool myFunc (int x, int y)
{
  bool result;
  int *pBufX = NULL;
  int *pBufY = NULL;

  /* do allocations here if possible */

  result = doStuff(x, y, pBufX, pBufY); // actual algorithm

  free(pBufX);
  free(pBufY);

  return result;
}

Now you only need to return from doStuff upon error, without worrying about deallocation. Ideally, you would do the malloc inside the wrapper function and thereby separate memory allocation from the actual algorithm, but sometimes that's not possible

Note that free guarantees that it does nothing in case you pass a null pointer to it.

EDIT:

Note that if you do the allocations inside doStuff you need to pass pointer-to-pointers!

Recommended way to handle multiple malloc errors in a single function in C

4 Answers4