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

Answer 1

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

Answer 2

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.