CODEWITHSUNDEEP
cmalloc
Saser
Saser

Reputation: 1

Is it a bad idea to check whether malloc was successful using a user-defined function?

When using malloc in C, I'm always told to check to see if any errors occurred by checking if it returned a NULL value. While I definitely understand why this is important, it is a bit of a bother constantly typing out 'if' statements and whatever I want inside them to check whether the memory was successfully allocated for each individual instance where I use malloc. To make things quicker, I made a function as follows to check whether it was successful. 

#define MAX 25
char MallocCheck(char* Check);
char *Option1, *Option2;
int main(){
    Option1 = (char *)malloc(sizeof(char) * MAX);
    MallocCheck(Option1);
    Option2 = (char *)malloc(sizeof(char) * MAX);
    MallocCheck(Option2);
    return 0;
}

char MallocCheck(char* Check){
    if(Check == NULL){
        puts("Memory Allocation Error");
        exit(1);
    }
}

However, I have never seen someone else doing something like this no matter how much I search so I assume it is wrong or otherwise something that shouldn't be done.

Is using a user-defined function for this purpose wrong and if so, why is that the case?

Upvotes: 0

Views: 989

Answers (2)

Myst
Myst

Reputation: 19221

This is an addendum to @chux's answer and the comments.

As stated, DRY code is generally a good thing and malloc errors are often handled the same way within a specific implementation.

It's true that some systems (notably, Linux) offer optimistic malloc implementations, meaning malloc always returns a valid pointer (never NULL) and the error is reported using a signal the first time data is written to the returned pointer... which makes error handling slightly more complex then the code in the question.

However, moving the error check to a different function might incur a performance penalty, unless the compiler / linker catches the issue and optimizes the function call away.

This is a classic use case for inline functions (on newer compilers) or macros.

i.e.

#include <signal.h>

void handle_no_memory(int sig) {
  if (sig == SIGSEGV) {
    perror("Couldn't allocate or access memory");
    /* maybe use longjmp to stay in the game...? Or not... */
    exit(SIGSEGV);
  }
}

/* Using a macro: */
#define IS_MEM_VALID(ptr)                                                      \
  if ((ptr) == NULL) {                                                         \
    handle_no_memory(SIGSEGV);                                                 \
  }
/* OR an inline function: */
static inline void *is_mem_valid(void *ptr) {
  if (ptr == NULL)
    handle_no_memory(SIGSEGV);
  return ptr;
}

int main(int argc, char const *argv[]) {
  /* consider setting a signal handler - `sigaction` is better, but I'm lazy. */
  signal(SIGSEGV, handle_no_memory);
  /* using the macro */
  void *data_macro = malloc(1024);
  IS_MEM_VALID(data_macro);
  /* using the inline function */
  void *data_inline = is_mem_valid(malloc(1024));
}

Both macros and inline functions prevent code jumps and function calls, since the if statement is now part of the function instead of an external function.

When using inline, the compiler will take the assembly code and place it within the function (instead of performing a function call). For this, we must trust the compiler to so it's job properly (it usually does it's job better than us).

When using macros, the preprocessor takes care of things and we don't need to trust the compiler.

In both cases the function / macro is local to the file (notice the static key word), allowing any optimizations to be performed by the compiler (not the linker).

Good luck.

Upvotes: 1

chux
chux

Reputation: 153338

Error checking is a good thing.

Making a helper function to code quicker, better is a good thing.

The details depend on coding goals and your group's coding standards.

OP's approach is not bad. I prefer to handle the error with the allocation. The following outputs on stderr @EOF and does not complain of a NULL return when 0 bytes allocated (which is not a out-of-memory failure).

void *malloc_no_return_on_OOM(size_t size) {
  void *p = mallc(size);
  if (p == NULL && size > 0) {
    // Make messages informative
    fprintf(stderr, "malloc(%zu) failure\n", size);
    // or 
    perror("malloc() failure");
    exit(1);
  }
  return p;
}

Advanced: Could code a DEBUG version that contains the callers function and line by using a macro.

Upvotes: 1

Related Questions