Decent ways to handle malloc failure?

Question

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 tree stores 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 malloc fails 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 returning NULL when it fails?

EDIT: I deeply appreciate all the answers and comments. Those were greatly helpful and interesting to me.

What do you ultimately want to do if a malloc fails? - In most cases I can think of the program should be terminated with a fatal error "out of memory". — nielsen
– nielsen, Commented Feb 7 at 14:02
if these three things have the same lifetime (i.e. allocated and freed at the same time) then I'd just allocate them together in one call to malloc. maybe put them into a struct so you don't need to do address calculations yourself, if you're finding that awkward, otherwise you could look at arena allocation — Sam Mason
– Sam Mason, Commented Feb 7 at 14:07
"this technique requires for NULL to be 0." The more important thing with that linked answer is that it tests if all pointers are NULL. It doesn't work to detect if at least one pointer is NULL which is what your code does. — Gerhardh
– Gerhardh, Commented Feb 7 at 14:08
Option 3 can be made less annoying by using int * for container and array indexing instead of getaddr (e.g. b = &container[1]). — zwol
– zwol, Commented Feb 7 at 14:29
Please edit your question and explain what you mean with "decent" or "nice" way. Avoiding code duplication? Doing the free at 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
– Bodo, Commented Feb 7 at 14:56

ikegami · Accepted Answer · 2025-02-14 02:11:13Z

9

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

Example of this approach in the wild.

edited Feb 14 at 2:11

answered Feb 7 at 14:37

ikegami

391k17 gold badges291 silver badges555 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Chris Feb 7 at 15:18

You may wish to explicitly call out the importance of initializing the pointers to NULL.

ikegami Feb 7 at 16:51

Added to my answer is an application of my answer to the newly-added code in the question.

Toby Speight · Accepted Answer · 2025-02-07 16:15:24Z

6

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 goto debate.

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.

edited Feb 7 at 16:15

Toby Speight

32.3k58 gold badges83 silver badges118 bronze badges

answered Feb 7 at 15:34

Lundin

220k47 gold badges282 silver badges446 bronze badges

3 Comments

chux Feb 7 at 15:48

int *a = NULL; int *b = NULL; int *c = NULL; --> Yes, one chunk of code to take objects from uninitialized to initialized (with no error possible). Lots of code to do whatever with the object set. And then "Cleanup happens the same whether or not any of the functions failed." --> Yes, one clean-up code. (IMO, this clean-up should be re-entrant.)

Toby Speight Feb 7 at 16:18

I'd argue that freeing is especially important when allocation fails, even if you intend to exit, as you don't know if your error handling might need to allocate. And the last time I needed to release resources like this in the right order, it was in a kernel module - in that context, shutting down is not an acceptable option - so being able to clean up properly is vital!

Toby Speight Feb 7 at 16:20

One possible improvement: don't attempt to allocate after the first failure. if ((a = malloc(sizeof *a)) && (b = malloc(sizeof *b)) && (c = malloc(sizeof *c))) - obviously use line-breaks to make this more comprehensible!

Michael Walsh Pedersen · Accepted Answer · 2025-02-07 15:23:06Z

4

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

answered Feb 7 at 15:23

Michael Walsh Pedersen

4893 silver badges6 bronze badges

Comments

dbush · Accepted Answer · 2025-02-07 15:51:28Z

2

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

}

edited Feb 7 at 15:51

answered Feb 7 at 15:32

dbush

233k27 gold badges261 silver badges334 bronze badges

2 Comments

Lundin Feb 7 at 15:36

One might want to drop an error message in a log, clean up other resources like file handles or threads etc.

John Bollinger Feb 7 at 15:36

Yes. Also, there are reasons why one might prefer abort()ing instead of exit(1)ing under these circumstances.

Vlad from Moscow · Accepted Answer · 2025-02-07 17:44:16Z

1

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.

edited Feb 7 at 17:44

answered Feb 7 at 15:19

Vlad from Moscow

313k27 gold badges204 silver badges358 bronze badges

Comments

Chris · Accepted Answer · 2025-02-07 21:22:39Z

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

Collectives™ on Stack Overflow

Decent ways to handle malloc failure?

6 Answers 6

2 Comments

3 Comments

Comments

2 Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

2 Comments

3 Comments

Comments

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related