Is this a good practice of freeing dynamically allocated memory or it's not?

Question

I wrote the following code sample:

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

char *test(void);

int main()
{
    char *base_ptr = NULL;

    base_ptr = test();

    for (char i = 0; i < 5; i++)
    {
        printf("base_ptr[%hhi] = %hhi\n", i, base_ptr[i]);
    }

    free(base_ptr);
    
    return 0;
}

char *test(void)
{
    char *local_ptr = NULL;


    local_ptr = (char *)malloc(5*sizeof(char));

    for (char i = 0; i < 5; i++)
    {
        scanf(" %hhi", &local_ptr[i]);
    }

    return local_ptr;
}

So, I know that once allocated by "malloc()" or "calloc()", I have to free the allocated memory using the "free()" function.

In the code sample I'm showing, I'm doing the allocation in the function "test", which returns a pointer. The pointer returned carries the base address of the allocated array. Inside the function "test()" there is no use of the function "free()", since reaching the return operator, the program leaves the function, which leads to freeing the memory as from the function itself, so from all of it's local variables, including the pointer, which holds the base address.

But inside the function "main()", I'm keeping that address in the pointer "base_ptr". I'm printing all the values, which I assigned in the already terminated function "test()", then I'm freeing the base adddress, using the function "free()".

I have a couple of questions regarding this.

Does this way of freeing alocated memory creates risk of memory leak, is it a good practice at all?

Is freeing dynamically allocated memory via function end or return the same as "free()" function?

If the memory, occupied (and initialized) by the function "test()" is freed due to it's execution end, isn't dangerous to access it's addresses in such maner, as in the code sample?

It is very valid to return a malloc allocated pointer and expect the user to free it afterwards. But, you have to make that very clear that this is the case in the documentation. — Pablo, May 13 '23 at 22:16
You need to error-check calls to `malloc` to be sure that the allocation did not fail. More idiomatic code might look like `char *local_ptr = malloc(sizeof *local_ptr * 5);`, i.e., no need to cast the result of `malloc`, and better to avoid explicit types in `malloc` calls. Then you need to check, e.g., `if (local_ptr) { /* do stuff */ } else { /* handle error */ }`. If it is a library function that returns an allocation, then the library needs to also supply a deallocation function since user code may not be compatible with library allocators. — ad absurdum, May 13 '23 at 22:36
I think in C it's more common for the _calling_ function to allocate and deallocate all necessary memory buffers, call the `test` function, and then free the memory. That way all allocation happen in the same function, or at least the same piece of code, it may look more symmetrical. — yeputons, May 14 '23 at 00:13

chux - Reinstate Monica · Accepted Answer · 2023-05-14T08:47:00.663

Is this a good practice of freeing dynamically allocated memory or it's not?

Yes. It is good practice.

Code checked out memory. Put it away when done .

"freed due to it's execution end" is a poor excuse. It is easier to port code to larger tasks that do not rely on that.

test() should clearly document that it is returning allocated memory that later needs a free().

These do not change the above answer, yet the troubles in OP's sample code belie other issues.

Code does have other weak or poor practices:

Lack of error check

Better code tests if malloc() succeeded.

Cast not needed

Castling the return value of malloc() is not needed.

Failure to check the return value of scanf().

Better code checks the return value.

Size to the referenced object, not type

Assign at declaration

//  char *local_ptr = NULL;
// local_ptr = (char *)malloc(5*sizeof(char));

char *local_ptr = malloc(sizeof local_ptr[0] * 5);

Pedantic: Use matchings specifier

%hhi matches a signed char *. It might fail for a char *.

Space is redundant

scanf("%hhi",... performs like scanf(" %hhi",... as %hhi skips leading white-space.

score 2 · Answer 2 · edited May 14 '23 at 00:08

But inside the function "main()", I'm keeping that address in the pointer "base_ptr". I'm printing all the values, which I assigned in the already terminated function "test()", then I'm freeing the base adddress, using the function "free()".

Returning from the function does not free the memory allocated via malloc family function. Those memory blocks have a lifetime the same as the program unless you free them yourself. In your function, you do not return a local pointer, only the reference to the allocated memory and this reference is stored in that local pointer.

Automatic storage duration objects instead have a lifetime same as enclosing scope and they stop to exist when you leave this scope

Example:

int *foo(void)
{
    int a[10];
    return a;  //returning pointer to the object which lifetime will end when 
               //function returns. If you use this returned value 
               //it will invoke undefined behaviour
}

Is freeing dynamically allocated memory via function end or return the same as "free()" function?

No, you need to free it yourself by calling free function.

Does this way of freeing alocated memory creates risk of memory leak, is it a good practice at all?

No, there is no risk of using free, and it is a very good practice

If the memory, occupied (and initialized) by the function "test()" is freed due to it's execution end, isn't dangerous to access it's addresses in such maner, as in the code sample?

This memory is not freed when you return from the function and it is valid until you 'free' it yourself.

Modern operating systems free the dynamically allocated memory on program termination, but it a good practice to free them.

Thanks. I was just afraid if this type of passing poitner, would lead to "dangling pointer" problem. — Sandrious, May 14 '23 at 07:36

Harith · Answer 3 · 2023-05-14T01:18:16.823

Yes, although most modern operating systems will free the memory on program termination, that isn't an excuse for having memory leaks all over one's code.

Also note that the return value of malloc() and family need not be cast. These functions return a generic pointer type (void *) that is implicitly converted to any other pointer type. As such, it is redundant and only serves to clutter one's code.

#if 0
    char *local_ptr = NULL;
    local_ptr = (char *)malloc(5*sizeof(char));
#else
    char *local_ptr = malloc (5);
    if (!local_ptr) {
        complain();
    }
#endif

We do not need sizeof(char) as it's defined by the standard to be 1 and there is no point in first declaring and initialising the pointer to NULL, only to overwrite it with the return of malloc() immediately.

Is this a good practice of freeing dynamically allocated memory or it's not?

3 Answers3