7
\$\begingroup\$

In order to concisify the handling of a failed call to malloc, realloc, or calloc, I have made a few files to dictate "safe" memory management. This is part of my project stac, so the prefix will appear in the following code snippets. Further, I have made the decision to use snake_case as opposed to standard camelCase.

What I hope to get out of this, and questions I have: are these functions in bad practice? Is there a different way I should be handling failed calls to memory allocators? And is there anything particularly wrong with the code I have written?

safe_mem.c

#include "safe_mem.h"

void* safe_malloc(size_t size){
    void* memory = malloc(size);

    if(memory == NULL){
        free(memory);
        runtime_error("failed to allocate memory.");
    }

    return memory;
}

void* safe_calloc(size_t num, size_t size){
    void* memory = calloc(num, size);

    if(memory == NULL){
        free(memory);
        runtime_error("failed to allocate memory.");
    }

    return memory;
}

void* safe_realloc(void* ptr, size_t size){
    void* memory = realloc(ptr, size);

    if(memory == NULL){
        free(memory);
        runtime_error("failed to reallocate memory.");
    }

    return memory;
}

safe_mem.h

#ifndef STAC_SAFE_MEM
#define STAC_SAFE_MEM
#include <stdio.h>
#include <stdlib.h>
#include "error.h"

void* safe_malloc(size_t);
void* safe_calloc(size_t, size_t);
void* safe_realloc(void*, size_t);
#endif

error.c

#include "error.h"

void make_error(char* type, char* message){
    eprintf("%s error: %s\n", type, message);
}

void fatal_error(char* type, char* message, int exit_code){
    make_error(type, message);
    exit(exit_code);
}

void runtime_error(char* message){
    fatal_error("Runtime", message, STATUS_RUNTIME_ERROR);
}

void generic_error(char* message){
    fatal_error("Generic", message, STATUS_GENERIC_ERROR);
}

error.h

#ifndef STAC_ERROR
#define STAC_ERROR
#include <stdio.h>
#include <stdlib.h>

#define eprintf(...) fprintf(stderr, __VA_ARGS__)
#define STATUS_OKAY (0)
#define STATUS_RUNTIME_ERROR (1)
#define STATUS_GENERIC_ERROR (-1)

void make_error(char*, char*);
void fatal_error(char*, char*, int);

void runtime_error(char*);
void generic_error(char*);

#endif
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't see the point in freeing memory that couldn't be allocated in all your allocation calls - Although freeing a NULL pointer doesn't normally hurt, it also doesn't do anything \$\endgroup\$ – tofro Jun 22 '17 at 7:24
  • \$\begingroup\$ @tofro hah, that's a good point... D'oh :p \$\endgroup\$ – Conor O'Brien Jun 22 '17 at 7:25
  • \$\begingroup\$ What's "standard" about camelCase? I don't think there's any camelCase in namespace std... \$\endgroup\$ – Toby Speight Jun 23 '17 at 12:54
  • \$\begingroup\$ I dislike the name "safe" - that's only the case if exiting the process is somehow safer than checking for NULL - and that's patently not the case for many programs. malloc_or_die() (and similar) would be much more honest! \$\endgroup\$ – Toby Speight Jun 23 '17 at 12:56
  • \$\begingroup\$ @TobySpeight I've never looked deeply into case convention in C, but I rather assumed it to be like C++. Apparently not. As for your second point, I don't see any alternative other than exiting the program. How could one recover from a memory shortage in a program? \$\endgroup\$ – Conor O'Brien Jun 23 '17 at 17:34
7
\$\begingroup\$

are these functions in bad practice?

I would say not, as long as the original functions they are wrapping have the same function signature to allow their replacement.

is there anything particularly wrong with the code I have written?

Allocating 0 bytes may return NULL, which does not indicate Out-of-memory.
Same for calloc(), realloc().

void* memory = malloc(size);
// if(memory == NULL){
if(memory == NULL && size > 0){

No point in calling free(memory) after if(memory == NULL)

if(memory == NULL){
  //  free(memory);

With eprintf(...), consider passing __FUNC__, __LINE__ of the error source to aid in debugging. Or at least create unique error messages.

void* safe_malloc(size_t size){
   runtime_error("failed to malloc memory.");

void* safe_calloc(size_t num, size_t size){
    runtime_error("failed to calloc memory.");

In a couple places, use const to allow wider application of the functions that are not changing the referenced data.

// void make_error(char* type, char* message){
void make_error(const char* type, const char* message){

I am not a fan of your name-space choices for error.h which includes eprintf, STATUS_OKAY, STATUS_GENERIC_ERROR, make_error, ... If I say these used in other ..c files how would I be guided back to error.h/error.c as their origin? Although a bit verbose, I prefer error_printf, ERROR_STATUS_GENERIC,...

Minor: #include <stdio.h> need not be in safe_mem.h nor #include <stdlib.h> in error.h. error.c should have #include <stdlib.h> as it calls exit(). error.c should not rely on error.h including that. Similar issues for safe_mem.c


From a design standpoint, I would also include void safe_free(void *) as the functionality of these safe functions may expand and require that even if it present does nothing. It makes safe_...() symmetric.

\$\endgroup\$
  • \$\begingroup\$ I used STATUS as a specifier. You would suggest changing the specifier to ERROR, so wouldn't ERROR_GENERIC be a better name? \$\endgroup\$ – Conor O'Brien Jun 23 '17 at 0:52
  • 1
    \$\begingroup\$ @CᴏɴᴏʀO'Bʀɪᴇɴ Yes. \$\endgroup\$ – chux Jun 26 '17 at 23:50
6
\$\begingroup\$
  • Only #include what you need. safe_mem.h doesn't require any file it includes. On the other hand, safe_mem.c does require error.h and stdlib.h.

    Notice that where stdlib.h is included is important. The client shouldn't care whether safe_mem.c implementation depends on stdlib.h or not. If it is exposed through the safe_mem.h, then changing safe_mem.c may require you to fix safe_mem.h and recompile anything which depends on it.

    Along the same line, I recommend to move #include <stdio.h> #define eprintf to error.c (unless there is a compelling reason to export this name), and drop #include <stdlib.h>.

  • safe_realloc is not really safe, because it will lead to memory leaks on realloc failure. Please disregard.

  • Bind * to the variable, not to the type:

    void *safe_malloc(....);
    
\$\endgroup\$
  • 1
    \$\begingroup\$ The program is done in on allocation failure, so not freeing memory would be immaterial anyway. \$\endgroup\$ – Deduplicator Jun 22 '17 at 0:32
  • 1
    \$\begingroup\$ @Deduplicator Agreed. I felt that quitting program in this case is too cruel, and didn't pay proper attention. Fixed \$\endgroup\$ – vnp Jun 22 '17 at 0:34
  • \$\begingroup\$ About your last point, I've seen it both ways. Is this a strict mandate? I personally find it more intuitive that the * is bound to the type rather than the variable. \$\endgroup\$ – Conor O'Brien Jun 22 '17 at 1:27
  • \$\begingroup\$ @CᴏɴᴏʀO'Bʀɪᴇɴ No, there is no mandate. It is just more consistent. Consider however a declaration int x, *y;. \$\endgroup\$ – vnp Jun 22 '17 at 2:03
  • \$\begingroup\$ @CᴏɴᴏʀO'Bʀɪᴇɴ Concerning --> * vs. -->* : the C spec uses void *malloc(size_t size); \$\endgroup\$ – chux Jun 22 '17 at 22:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.