CODEWITHSUNDEEP
cerror-handlingmalloc
Anusha Dharmasena
Anusha Dharmasena

Reputation: 1269

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

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

Upvotes: 6

Views: 804

Answers (4)

Jubatian
Jubatian

Reputation: 2191

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.

Upvotes: 1

Lundin
Lundin

Reputation: 213989

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!

Upvotes: 0

Jabberwocky
Jabberwocky

Reputation: 50832

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

Upvotes: 4

William Pursell
William Pursell

Reputation: 212288

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.

Upvotes: 1

Related Questions