Suppose that I need to malloc three times and I would like to make an early return if one of the malloc calls have failed.
My first try:
void test(void) {
int *a, *b, *c;
a = malloc(sizeof(*a));
if (a == NULL)
return;
b = malloc(sizeof(*b));
if (b == NULL) {
free(a); /* since `a` has been `malloc`ed. */
return;
}
c = malloc(sizeof(*c));
if (c == NULL) {
free(a); // since `a` and `b` have
free(b); // been `malloc`ed.
return;
}
}
However, somehow I don't think this is a nice way to handle malloc failures.
My second thought:
a = malloc(sizeof(*a));
b = malloc(sizeof(*b));
c = malloc(sizeof(*c));
if (a == NULL || b == NULL || c == NULL) {
free(a); // I've read that it's fine
free(b); // to `free` a NULL pointer.
free(c);
return;
}
I think that this is better in that it looks clean, but that's because I'm handling only three pointers. If I had more pointers, it wouldn't look clean.
Meanwhile, I've seen a brilliant method which casts pointers into uintptr_t. However, as far as I understand, this technique requires for NULL to be 0.
My last resort:
void *container;
container = malloc(sizeof(int) * 3);
if (container == NULL)
return;
a = getaddr(0);
b = getaddr(1);
c = getaddr(2);
// def: getaddr
static inline int *getaddr(void *base, int index) {
return base + (sizeof(int) * index);
}
By mallocing the total memory I needed, I was able to write the NULL test only once, but it's still a headache to me to do so.
So I wonder whether there are nicer ways or best practices to handle this situation. It would be greatly appreciated if you could enlighten me.
EDIT: First of all, I thank all of you for helping me out. Please let me elaborate the intention of this question.
- What I meant by "decent" ways is that I would like to write an "easy-to-read" and "easy-to-write" code, i.e. a concise code, rather than an "optimized" code.
- The reason I need three pointers is that I'm trying to make a tree data structure like the below--
struct treestores three pointers which are dynamically allocated:
typedef struct tree tree_t;
struct tree {
tree_t *parent; // a pointer to the parent tree.
list_t *children; // a pointer to the list which has the pointers to the child trees of this tree.
// list_t is a linked list I've made.
void *data; // a pointer to the data this tree has.
size_t data_size; // the size of the data.
};
// This function creates a new tree object.
// This function returns a pointer to the newly created tree.
// If failed, returns NULL.
extern tree_t *tree_plant(void *data, size_t data_size) {
// under these conditions, fails to create one.
if (data == NULL || data_size == 0)
return NULL;
tree_t *sapling;
void *tree_data;
list_t *tree_children;
// This is the very point which confuses me.
// I would like to return NULL if failed to malloc.
sapling = malloc(sizeof(tree_t));
if (sapling == NULL)
return NULL;
tree_data = malloc(data_size);
if (tree_data == NULL) {
free(sapling);
return NULL;
}
tree_children = list_create(); // list_create makes a new list.
if (tree_children == NULL) { // list_create returns NULL if it fails to make one, like tree_plant does.
free(sapling);
free(tree_data);
return NULL;
}
// Initializes all fields
sapling->parent = NULL;
sapling->children = tree_children;
sapling->data = tree_data;
sapling->data_size = data_size;
// I've heard it's best practice to copy data when implementing an abstract data type.
memcpy(sapling->data, data, data_size);
// success to make a tree
return sapling;
}
// This is the list implementation.
typedef struct node {
struct node *prev;
struct node *next;
void *data;
size_t data_size;
} node_t;
typedef struct list list_t;
struct list {
node_t *head;
node_t *tail;
size_t size; // the length of this list.
};
extern list_t *list_create(void) {
list_t *list;
list = malloc(sizeof(list_t));
if (list == NULL)
return NULL;
list->head = NULL;
list->tail = NULL;
list->size = 0;
return list;
}
- Given this situation, what I want to do when
mallocfails is an early return, and the reason I want the early return is, I'm afraid, I don't know. Perhaps because I want a kind of consistency(?) with functions returningNULLwhen it fails?
EDIT: I deeply appreciate all the answers and comments. Those were greatly helpful and interesting to me.
Suppose that I need to malloc three times and I would like to make an early return if one of the malloc calls have failed.
My first try:
void test(void) {
int *a, *b, *c;
a = malloc(sizeof(*a));
if (a == NULL)
return;
b = malloc(sizeof(*b));
if (b == NULL) {
free(a); /* since `a` has been `malloc`ed. */
return;
}
c = malloc(sizeof(*c));
if (c == NULL) {
free(a); // since `a` and `b` have
free(b); // been `malloc`ed.
return;
}
}
However, somehow I don't think this is a nice way to handle malloc failures.
My second thought:
a = malloc(sizeof(*a));
b = malloc(sizeof(*b));
c = malloc(sizeof(*c));
if (a == NULL || b == NULL || c == NULL) {
free(a); // I've read that it's fine
free(b); // to `free` a NULL pointer.
free(c);
return;
}
I think that this is better in that it looks clean, but that's because I'm handling only three pointers. If I had more pointers, it wouldn't look clean.
Meanwhile, I've seen a brilliant method which casts pointers into uintptr_t. However, as far as I understand, this technique requires for NULL to be 0.
My last resort:
void *container;
container = malloc(sizeof(int) * 3);
if (container == NULL)
return;
a = getaddr(0);
b = getaddr(1);
c = getaddr(2);
// def: getaddr
static inline int *getaddr(void *base, int index) {
return base + (sizeof(int) * index);
}
By mallocing the total memory I needed, I was able to write the NULL test only once, but it's still a headache to me to do so.
So I wonder whether there are nicer ways or best practices to handle this situation. It would be greatly appreciated if you could enlighten me.
EDIT: First of all, I thank all of you for helping me out. Please let me elaborate the intention of this question.
- What I meant by "decent" ways is that I would like to write an "easy-to-read" and "easy-to-write" code, i.e. a concise code, rather than an "optimized" code.
- The reason I need three pointers is that I'm trying to make a tree data structure like the below--
struct treestores three pointers which are dynamically allocated:
typedef struct tree tree_t;
struct tree {
tree_t *parent; // a pointer to the parent tree.
list_t *children; // a pointer to the list which has the pointers to the child trees of this tree.
// list_t is a linked list I've made.
void *data; // a pointer to the data this tree has.
size_t data_size; // the size of the data.
};
// This function creates a new tree object.
// This function returns a pointer to the newly created tree.
// If failed, returns NULL.
extern tree_t *tree_plant(void *data, size_t data_size) {
// under these conditions, fails to create one.
if (data == NULL || data_size == 0)
return NULL;
tree_t *sapling;
void *tree_data;
list_t *tree_children;
// This is the very point which confuses me.
// I would like to return NULL if failed to malloc.
sapling = malloc(sizeof(tree_t));
if (sapling == NULL)
return NULL;
tree_data = malloc(data_size);
if (tree_data == NULL) {
free(sapling);
return NULL;
}
tree_children = list_create(); // list_create makes a new list.
if (tree_children == NULL) { // list_create returns NULL if it fails to make one, like tree_plant does.
free(sapling);
free(tree_data);
return NULL;
}
// Initializes all fields
sapling->parent = NULL;
sapling->children = tree_children;
sapling->data = tree_data;
sapling->data_size = data_size;
// I've heard it's best practice to copy data when implementing an abstract data type.
memcpy(sapling->data, data, data_size);
// success to make a tree
return sapling;
}
// This is the list implementation.
typedef struct node {
struct node *prev;
struct node *next;
void *data;
size_t data_size;
} node_t;
typedef struct list list_t;
struct list {
node_t *head;
node_t *tail;
size_t size; // the length of this list.
};
extern list_t *list_create(void) {
list_t *list;
list = malloc(sizeof(list_t));
if (list == NULL)
return NULL;
list->head = NULL;
list->tail = NULL;
list->size = 0;
return list;
}
- Given this situation, what I want to do when
mallocfails is an early return, and the reason I want the early return is, I'm afraid, I don't know. Perhaps because I want a kind of consistency(?) with functions returningNULLwhen it fails?
EDIT: I deeply appreciate all the answers and comments. Those were greatly helpful and interesting to me.
Share Improve this question edited Feb 8 at 11:13 Doohyeon Won asked Feb 7 at 13:50 Doohyeon WonDoohyeon Won 3292 silver badges13 bronze badges 12 | Show 7 more comments6 Answers
Reset to default 8You can pass NULL to free, so no checks are necessary.
tree_t *tree_plant( void *data, size_t data_size ) {
// ...
tree_t *sapling = NULL;
void *tree_data = NULL;
sapling = malloc( sizeof( tree_t ) );
if ( !sapling )
goto ERROR;
tree_data = malloc( data_size );
if ( !tree_data )
goto ERROR;
list_t *tree_children = list_create();
if ( !tree_children )
goto ERROR;
// ...
return sapling;
ERROR:
free( tree_data );
free( sapling );
return NULL;
}
The above is specialization of the following general approach which I find clearer:
tree_t *tree_plant( void *data, size_t data_size ) {
// ...
tree_t *sapling = malloc( sizeof( tree_t ) );
if ( !sapling )
goto ERROR1;
void *tree_data = malloc( data_size );
if ( !tree_data )
goto ERROR2;
list_t *tree_children = list_create();
if ( !tree_children )
goto ERROR3;
// ...
return sapling;
ERROR3:
free( tree_data );
ERROR2:
free( sapling );
ERROR1:
return NULL;
}
The very obvious construction-destruction symmetry with minimal code and minimal indenting is what makes this approach very clean.
Furthermore, this approach is incredibly flexible without sacrificing any readability.
- It allows error-specific code.
- It can be used to perform any kind of post-processing, and it does so out of the way of the main flow of the function.
- It can perform the post-processing conditionally (e.g. on error) or not.
bool test( void ) {
bool rv = false;
FILE *f1 = fopen( ... );
if ( !f1 ) {
// Output relevant error message here.
goto ERROR1;
}
FILE *f2 = fopen( ... );
if ( !f2 ) {
// Output relevant error message here.
goto ERROR2;
}
// Do something with file handles here.
rv = true;
fclose( f2 );
ERROR2:
fclose( f1 );
ERROR1:
return rv;
}
This has been discussed here many times before. My general answer is to keep resource allocation and algorithm separated in two functions, where the former calls the latter:
void test(void)
{
int *a = NULL;
int *b = NULL;
int *c = NULL;
a = malloc(sizeof *a);
b = malloc(sizeof *b);
c = malloc(sizeof *c);
if(a && b && c)
{
actual_algorithm(a, b, c);
}
free(a);
free(b);
free(c);
}
This comes with the following advantages:
- Cleanup happens the same whether or not any of the functions failed.
- Always initializes all pointers to NULL -
free(NULL)is well-defined. - In case there are errors in
actual_algorithm, it can return and let the caller worry about resource clean-up. - Design-wise the algorithm should only concern itself with its actual task and not with "meta tasks" such as resource allocation.
- Less clean up clutter code in the actual algorithm which reduces readability. (Error handling tends to clutter down code quite a lot.)
- No tiresome
gotodebate.
We may also note that an array would have been much prettier and more convenient than 3 individual pointer variables.
However, in case of malloc() specifically, there's just so much you can do if it fails. Basically you just have to close up shop but do so in a controlled manner, maybe write errors to logs or clean other resources before the program shuts down. Therefore it isn't really important to free() everything upon malloc failure, since the OS will clean up that memory soon anyway, when we close the program.
Apart from that scenario, it is generally good practice to explicitly call free() rather than relying on the OS to do so, because when calling free() we expose heap corruption or pointer-related bugs early on. A crash during free() means there's a bug elsewhere in the program.
You could use, what I would call 'telescope' code:
int test(void) {
int *a, *b, *c;
int rc;
rc = 0;
a = malloc(sizeof(*a));
b = a ? malloc(sizeof(*b)) : NULL ;
c = b ? malloc(sizeof(*c)) : NULL ;
if ( c )
{
// do something with a, b and c
// return here?
}
if ( c ) free(c); else rc = -3;
if ( b ) free(b); else rc = -2;
if ( a ) free(a); else rc = -1;
return rc;
}
Given that there's not much you can do once malloc fails, you may just want to wrap malloc and have it clean up and exit if it fails:
void *safe_malloc(size_t size)
{
void *p = malloc(size);
if (p == NULL) {
perror("malloc failed!");
// perform cleanup, log, etc.
exit(1);
}
return p;
}
void test(void) {
int *a, *b, *c;
a = safe_malloc(sizeof(*a));
b = safe_malloc(sizeof(*b));
c = safe_malloc(sizeof(*c));
}
A simple way is to use loops for an array of pointers as for example
int *a = NULL, *b = NULL, *c = NULL;
int ** arr[] = { &a, &b, &c };
const size_t N = sizeof( arr ) / sizeof( *arr );
size_t i = 0;
while ( ( i < N ) && ( *arr[i] = malloc( sizeof( **arr[i] ) ) ) != NULL )
{
++i;
}
if ( i != N ) // Memory was not allocated for all pointers
{
while ( i != 0 )
{
free( *arr[--i] );
}
// possibly return from the function
}
// continue to work with pointers
With this approach you can deal with any number of pointers without changing the code except the initializing list of the array.
Pay attention to that in code in other ansswers if you will change the number of pointers then you will need to change the code in many places. Such a code is error prone.
A quickly mocked up suggestion, similar to dbush's wrapper around malloc but with a resizable array that stores pointers to allocated memory and can then "cleanup" by iterating over that array to free all of them in the event of a single failure.
#include <stdlib.h>
#include <stdio.h>
#define PP_MIN_CAP 16
typedef struct {
size_t cap;
size_t sz;
void **data;
} pointer_pool;
typedef void (*pp_err_handler)(pointer_pool *);
pointer_pool *pp_init(void) {
pointer_pool *pp = malloc(sizeof(*pp));
if (!pp) {
return NULL;
}
pp->cap = PP_MIN_CAP;
pp->sz = 0;
pp->data = malloc(sizeof(*pp->data) * pp->cap);
if (!pp->data) {
free(pp);
return NULL;
}
return pp;
}
void pp_cleanup(pointer_pool *pp) {
if (!pp) {
return;
}
fprintf(stderr, "Freeing %zu pointers\n", pp->sz);
for (size_t i = 0; i < pp->sz; i++) {
free(pp->data[i]);
}
free(pp);
}
void pp_cleanup_and_exit(pointer_pool *pp) {
pp_cleanup(pp);
exit(EXIT_FAILURE);
}
pointer_pool *pp_add_pointer(pointer_pool *pp, void *ptr, pp_err_handler hnd) {
if (!pp || !pp->data) {
return NULL;
}
if (pp->sz >= pp->cap) {
void **temp = realloc(pp->data, sizeof(*pp->data) * pp->cap * 2);
if (!temp) {
hnd(pp);
return NULL;
}
pp->data = temp;
pp->cap *= 2;
}
pp->data[pp->sz++] = ptr;
return pp;
}
void handle_error_with_cleanup(pointer_pool *pp) {
fprintf(stderr, "Memory allocation failure!\n");
pp_cleanup_and_exit(pp);
}
void *cust_malloc(size_t bytes, pointer_pool *pp, pp_err_handler hnd) {
void *mem = malloc(bytes);
if (!mem) {
hnd(pp);
return NULL;
}
pp_add_pointer(pp, mem, hnd);
return mem;
}
One test case:
int main(void) {
pointer_pool *pp = pp_init();
int *a = cust_malloc(sizeof(*a), pp, handle_error_with_cleanup);
int *b = cust_malloc(sizeof(*a), pp, handle_error_with_cleanup);
int *c = cust_malloc(sizeof(*a), pp, handle_error_with_cleanup);
int *d = cust_malloc(sizeof(*a) * 10000, pp, handle_error_with_cleanup);
printf("%p %p %p %p\n", a, b, c, d);
pp_cleanup(pp);
}
Output is:
0x600003604030 0x600003604050 0x600003604060 0x7f7e5d800000
Freeing 4 pointers
But if we increase the size of the allocations to break malloc intentionally...
int main(void) {
pointer_pool *pp = pp_init();
int *a = cust_malloc(sizeof(*a), pp, handle_error_with_cleanup);
int *b = cust_malloc(sizeof(*a), pp, handle_error_with_cleanup);
int *c = cust_malloc(sizeof(*a), pp, handle_error_with_cleanup);
int *d = cust_malloc(sizeof(*a) * 1000000000000000000, pp, handle_error_with_cleanup);
printf("%p %p %p %p\n", a, b, c, d);
pp_cleanup(pp);
}
Output:
Memory allocation failure!
Freeing 3 pointers
mallocfails? - In most cases I can think of the program should be terminated with a fatal error "out of memory". – nielsen Commented Feb 7 at 14:02NULL. It doesn't work to detect if at least one pointer isNULLwhich is what your code does. – Gerhardh Commented Feb 7 at 14:08int *forcontainerand array indexing instead ofgetaddr(e.g.b = &container[1]). – zwol Commented Feb 7 at 14:29freeat only one place? You should write code that is easy to read, not try to optimize it. (In most cases the compiler will optimize the code better than you.) And tell us why you (think you) need 3 separately allocated blocks or 3 pointers. Showing what you want to do with the pointers might help to understand your use case. – Bodo Commented Feb 7 at 14:56