Recommended way to handle multiple malloc errors in one function in C

Question

What is the recommended way to handle multiple malloc errors that can occur 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 mullocks is large, you can forget to free them and cause a memory leak. In addition, if there is some kind of log that should be displayed when an error occurs, the code becomes very long.

I saw code that processes them with goto, where you clear all 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 with the design in the first place. Is there a thumb rule when designing functions when it comes to minimizing a few mallocs?

Edit: There is another way that I saw and used. Instead of using goto, you save the program status and continue to work only if the status is in order. Like goto, but does not use goto. But this increases the number of if statements, which can slow down the code.

 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 recommend your second ("evil" avoiding) solution. This is more complicated than goto solution.

BTW is safe to free the null pointer, so

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

can be replaced by:

  free(pBufY); 

Answer 2

Personally, I prefer to use goto. But since it is safe to be free(NULL) , you can usually do all the distributions ahead:

 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 allocated memory for calculation in order to get the size for later allocation, your function is probably quite complicated, and in any case, it should be reorganized.

Answer 3

Great question! (I thought I would find a hoax, but no, strange, such an important aspect in C that seems to have never been asked before)

There are two questions that I found for some of this field:

• Valid use of goto for error management in C?
• Any good idioms for handling errors in direct C programs?

They are mainly focused on the goto path. So first allow it.

Goto method

If you have code that depends on the allocation of several resources that are subsequently to be released, you can use the template as shown 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 can read more about this approach in the excellent Regehr blog: http://blog.regehr.org/archives/894 , also pointing to the Linux kernel, which often uses this template.

Arrow code

This is one possible way to do it differently. The above example looks like this: arrow:

 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 that the template got its name with is the potentially deep embedding (code similar to an arrow), why this template is so disliked.

other methods

I can’t think about the need to allocate and free resources (with the exception of the flag option that you describe in detail in your question). You have more freedom, if you do not have such a restriction, you can see some good approaches described in other questions and answers.

If the resources are independent of each other, you can also use other templates (for example, the early return that your first code example provides), however these two are the ones that can work correctly with the resources if you need it (i.e. resource2 can be assigned only on top of a valid resource1), and if the free function of this resource does not handle an unsuccessful return return.

Answer 4

The most correct way to do this, in my opinion, is to transfer 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 after an error, without worrying about freeing it. Ideally, you should make malloc inside the wrapper function and thereby allocate memory allocation from the actual algorithm, but sometimes this is not possible

Please note that there are free guarantees that it does nothing if you pass it a null pointer.

EDIT:

Note that if you are doing a selection inside doStuff , you need to pass pointers to pointers!