Thread: Freeing memory before exit

Is it a good practice to free memory before exiting the program? I've read many places online that since modern operating systems reclaim all resources before exiting, there's no need to free memory.

Consider the following example:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define NAME_LENGTH 30

int main(void) {
    char *name = malloc(NAME_LENGTH);
    if (name == NULL) {
        fputs("Not enough memory\n", stderr);
        exit(EXIT_FAILURE);
    }
    
    fputs("Enter your name: ", stderr);
    fflush(stderr);
    if (fgets(name, NAME_LENGTH, stdin) == NULL) {
        free(name);
        fputs("Either EOF was encountered and no bytes were read, or an error \
ocurred when reading from standard input\n", stderr);
        exit(EXIT_FAILURE);
    }
    
    char *newline = strchr(name, '\n');
    if (newline != NULL) {
        *newline = '\0';
    }
    
    printf("Hello %s\n", name);
    
    free(name);
    
    return 0;
}

Notice the two calls to free(). One before calling exit() if the fgets() function fails, and the other one before terminating the program.

Now, the latter is pretty simple to make. The former however, can be a little of a hassle especially if there are many aborting sections and memory allocated from different functions. Should I worry about freeing the memory even on complex scenarios?

Originally Posted by Salem

"All" is an over simplification.
If you're talking about modern desktop OS, then memory from using malloc is reclaimed.

Good point. Really, even though I asked about memory allocated, it could be any resource that need to be freed. FILE* streams returned by fopen() come to mind. For the sake of simplicity, let's assume only space allocated by functions like malloc(), realloc() or calloc() are resources needed to be freed.

Originally Posted by Salem

Put it this way, if I ran valgrind with your program and found no memory leaks, I would be a lot more confident that you'd been paying attention to all the details.

You can avoid a lot of the tediousness through careful arrangement of the code.

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
 
#define NAME_LENGTH 30

void dowork(char *name, size_t buflen) {
    fputs("Enter your name: ", stderr);
    fflush(stderr);
    if (fgets(name, buflen, stdin) == NULL) {
        fputs("Either EOF was encountered and no bytes were read, or an error"
              " ocurred when reading from standard input\n", stderr);
    } else {
        char *newline = strchr(name, '\n');
        if (newline != NULL) {
            *newline = '\0';
        }
        printf("Hello %s\n", name);
    }
}
 
int main(void) {
    char *name = malloc(NAME_LENGTH);
    if (name == NULL) {
        fputs("Not enough memory\n", stderr);
        exit(EXIT_FAILURE);
    } else {
        dowork(name, NAME_LENGTH);
        free(name);
    }
    return 0;
}

Valgrind is certainly a tool I definitely must check, because I'm currently using cppcheck to find errors in my code and it gives false positives sometimes, especially when I exit within the calls of other functions, like this:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define NAME_LENGTH 30

void exit_with_msg(const char *msg) {
    fprintf(stderr, "%s\n", msg);
    exit(EXIT_FAILURE);
}

int main(void) {
    char *name = malloc(NAME_LENGTH);
    if (name == NULL) {
        fputs("Not enough memory\n", stderr);
        exit(EXIT_FAILURE);
    }
    
    fputs("Enter your name: ", stderr);
    fflush(stderr);
    if (fgets(name, NAME_LENGTH, stdin) == NULL) {
        free(name);
        exit_with_msg("Either EOF was encountered and no bytes were read, or an"
                      "error ocurred when reading from standard input");
    }
    
    char *newline = strchr(name, '\n');
    if (newline != NULL) {
        *newline = '\0';
    }
    
    printf("Hello %s\n", name);
    
    free(name);
    
    return 0;
}

cppcheck prints the following:
warning: Memory pointed to by 'name' is freed twice. [doubleFree]

I guess it's because it doesn't keep track of functions that don't return to their caller.

By the way, I like the way you concatenate long strings instead of using the backslash like I usually do haha. I had no idea it could be done like that.

Also, totally unrelated to the topic and I have no intention to be nitpicking. If you define buflen as size_t and pass it to the size paramater of fgets() which is of type int, wouldn't that cause overflow on systems where SIZE_MAX > INT_MAX? Really, 30 is by far less than the limits defined by the standard so I know it's not a problem, it just caught my attention that you passed a different type.

Compiler Explorer can be enlightening.
Compiler Explorer


I used clang because it supports a lot more warnings than gcc and it has a -Weverything option to enable them all. As you can see the compiler doesn't complain about NAME_LENGTH being replaced with the explicit type cast ((size_t)30) by the preprocessor. It's an integer literal (constant at compile time) that the compiler reshapes to whatever suites, taking care that it doesn't break the logic.
I don't speak assembly (I wish I would). However, I'm able to understand it a little. There's a remarkable instruction in preparation of calling malloc:
mov edi, 30
The edi is a 32-bit register. So only the 32 low order bits of NAME_LENGTH are copied to the parameter of malloc, even though the parameter (must be rdi then) has a size of 64-bit in the x86-64 implementation.

Originally Posted by Salem

> warning: Memory pointed to by 'name' is freed twice. [doubleFree]
You could try tagging the function as __attribute__((__noreturn__)), so the tool knows that it never returns.

But it should have figured that much out through calling exit()

Nope. Adding _Noreturn gives the same output. Actually, I use Debian stable and the package is not the latest version available online, so come to think of it, maybe newer versions recognize this.

Originally Posted by Salem

> If you define buflen as size_t and pass it to the size paramater of fgets() which is of type int, wouldn't that cause overflow on systems where SIZE_MAX > INT_MAX?
Actually, I'd forgotten that fgets took an int. Pretty much everything else uses size_t to specify the length of a block of memory.

But if you're allocating >INT_MAX memory to pass to fgets(), you're doing it wrong anyway.

I tend to be more lenient about freeing memory when exiting the program through abnormal routes.

Yeah... Most functions take size_t when it comes to the object's size, but fgets() is one of the exceptions. But these kind of inconsistencies in favor of preserving legacy code is what makes C special

Originally Posted by aGerman

Compiler Explorer can be enlightening.
Compiler Explorer


I used clang because it supports a lot more warnings than gcc and it has a -Weverything option to enable them all. As you can see the compiler doesn't complain about NAME_LENGTH being replaced with the explicit type cast ((size_t)30) by the preprocessor. It's an integer literal (constant at compile time) that the compiler reshapes to whatever suites, taking care that it doesn't break the logic.

Hey, maybe I'm wrong, but I believe turning on absolutely all warnings in a compiler is probably not something you want. In fact, in the code you linked, it gives several errors that make zero sense to me.

<source>:7:6: warning: no previous prototype for function 'exit_with_msg' [-Wmissing-prototypes]
I don't see the point of this one. If the function were used without previously being declared I could see the problem. But yielding the error in the function declaration? I don't know what the intent of that warning is.

<source>:7:37: warning: function 'exit_with_msg' could be declared with attribute 'noreturn' [-Wmissing-noreturn]

<source>:27:11: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
The problem here is that it complains that the _Noreturn attribute could be added. OK, I buy it. But complaining afterwards that the code is incompatible with pre-C99 doesn't make sense, because you just told me to include an attribute that was introduced in C11. I mean, these two warnings could be useful if you use preprocessor directives and checking what version of the standard the compiler supports. But I don't know, I'm not going to put all variable declarations at the top of a block just to make it compatible with ancient and probably gone for good pre-C99 compilers

And that's exactly why GCC lacks an option to turn on all warnings. Because some are too specific, and others may be mutually exclusive.

Funnily enough, the noreturn attribute will be changed in C23 to the ugly (in my honest opinion) C++ style syntax [[noreturn]].

Originally Posted by aGerman

I don't speak assembly (I wish I would). However, I'm able to understand it a little. There's a remarkable instruction in preparation of calling malloc:
mov edi, 30
The edi is a 32-bit register. So only the 32 low order bits of NAME_LENGTH are copied to the parameter of malloc, even though the parameter (must be rdi then) has a size of 64-bit in the x86-64 implementation.

I wish I could, too.

Hey, maybe I'm wrong, but I believe turning on absolutely all warnings in a compiler is probably not something you want.

I absolutely agree! That was just to ensure we don't accidentally leave out any warning option related to an implicit type conversion.


By focusing on this assembly instruction, I wanted to emphasize that the compiler is perfectly capable of detecting the number of bits required to represent a given value, provided the value is known at compile time. So the compiler knows that value 30 fits well into 32 bits and has opted for the slightly cheaper 32-bit copy operation, although we have tried to supply a 64-bit value to the 64-bit parameter in C code.


I know this answers your question about the type overflow only indirectly. Perhaps a better answer is that you find the value 30 as a literal value in the related instructions (lines 19 and 41 of the ASM code). It's not that the value is first saved in a 64-bit register and later copied into 32-bit registers using a kind of narrowing operation. This would be a waste of ressources.


Just to prove that the compiler detects the size that a value occupies, update line 5 in the C code:
#define NAME_LENGTH ((size_t)0xffffffffffffffffULL)
Now you get the desired warning. (Besides of instantly running out of memory of course.)

Due to my poor ASM knowledge I don't understand why the high order bits of rdi are initialized. I would have expected to find a movsxd or something. But that's not the case.

Originally Posted by aGerman

Don't focus too much on signedness. It's rather about the method how signed integers are represented in a given implementation. The r?? are 64-bit, and the e?? are 32-bit registers. There is no such attribute like "signed" or "unsigned" for registers. As we are talking about two's complement to represent signed integers, the -1 just means all bits are set 1. Similarly, 4294967295 means 32 bits are set that represent -1 as a 32-bit signed integer.

Oh, OK. Like I said I have absolutely no idea about assembly... So I was just curious why they were passed like that.

Thank you very much Salem and aGerman for your help. I will do my best to keep track of resources and free them before terminating the program