Using malloc like this is correct?

Question

I am new to dynamic memory allocation. Does this type of use create any memory leak or unallocated memory access?

char *func2(char *str) {
    //Do something like writing 10 characters to str
    return str;
}

void func1() {
    char *str = NULL, *res;
    int len = 10;
    str = (char *)malloc(len * sizeof(char));
    res = func2(str);
    free(str); 
    printf("%s", res);
}

Does the final printf statement in func1 result in segmentation fault as str is already freed, will res be a dangling pointer, with no memory location to point?

You [should not cast the results of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/22538350#22538350). And in your example, "do something like add 10 characters to str" would likely overflow your buffer since you're only allocating a total of 10 chars to begin with. Finally, freeing an already freed pointer is bad. — lurker, Jun 22 '17 at 16:25
Yup, you'll get garbage. That's why [some even set the variable to `NULL` after it's been freed](https://stackoverflow.com/a/1025604/4520911) - it almost always forces the program to crash upon being accessed. — iRove, Jun 22 '17 at 16:37
Aside: consider `str = malloc(sizeof *str * len);` instead of `str = (char *) malloc(len * sizeof(char));`. Easier to code, review and maintain. — chux - Reinstate Monica, Jun 22 '17 at 18:43
@iRove: Of course, setting `str` to `NULL` after it is freed would make no difference here, because it wouldn't null `res`. — Fred Larson, Jun 22 '17 at 18:48
@FredLarson Well - yes...but I was just making a point to demonstrate the dangers of using _any_ variable after it's been freed (unless, of course, you reinitialize it). — iRove, Jun 22 '17 at 18:52

score 2 · Accepted Answer · answered Jun 22 '17 at 16:26

2

res is used after the memory it references is freed. This is undefined behavior, which may or may not lead to a runtime error. Undefined behavior is undefined, after all.

What I think is likely to happen, in practice, is that your program will happily print whatever is in the memory res points to. That memory isn't gone, you've just said you wouldn't use it anymore. What will be in that memory is uninitialized garbage, since you never wrote anything to it. I think you'll get gobbledygook for output.

answered Jun 22 '17 at 16:26

Fred Larson

60,987
18
112
174

So I should free str only after usage of res is completed? – deathstroke05 Jun 23 '17 at 04:46
`void func1() { char *str = NULL, *res; int len = 10; res = func2(str, len); printf("%s",res); }` Now i want func2 to malloc for length 'len' , write some characters to `str`, make `res` point to `str`, free `str` and return res. How would I do this? – deathstroke05 Jun 23 '17 at 04:58
I'm not sure why you'd even bother with `str` to do that. Why not just `malloc`, fill, and return the result? It would probably look similar to [`strdup`](https://stackoverflow.com/q/252782/10077). – Fred Larson Jun 23 '17 at 13:46
Actually, the memory *might* be gone (unmapped), depending on how `free()` is implemented on the target platform. But what you say is the *likely* outcome on systems that are widely-used at the time of writing. – Toby Speight Jun 27 '17 at 16:39

score 1 · Answer 2 · answered Jun 23 '17 at 13:15

Yes, it's an error. We can demonstrate the error by turning the code into a working example, and running it under Valgrind:

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

char *func2(char *str)
{
    strcpy(str, "hello!");
    return str;
}

int main()
{
    char *str, *res;
    int len = 10;
    str = malloc(len);
    res = func2(str);
    free(str);
    printf("%s", res);
}

The output shows us what happened:

gcc -std=c11 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds      44704791.c    -o 44704791
valgrind --leak-check=full ./44704791 
==12624== Memcheck, a memory error detector
==12624== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==12624== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==12624== Command: ./44704791
==12624== 
==12624== Invalid read of size 1
==12624==    at 0x4C2EDA2: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12624==    by 0x4E80D77: vfprintf (vfprintf.c:1637)
==12624==    by 0x4E871F8: printf (printf.c:33)
==12624==    by 0x1087B5: main (44704791.c:18)
==12624==  Address 0x51d7040 is 0 bytes inside a block of size 10 free'd
==12624==    at 0x4C2CDDB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12624==    by 0x10879D: main (44704791.c:17)
==12624==  Block was alloc'd at
==12624==    at 0x4C2BBAF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12624==    by 0x10877D: main (44704791.c:15)

It shows us that the printf at line 18 tried to read from memory that was released at line 17. Additionally, it shows that the memory in question was allocated at line 15.

The moral of the story is that you should release memory when you have finished using it (not before), and that can be difficult to do if you are not clear about whether your functions take ownership of memory passed to them in pointers.

Using malloc like this is correct?

2 Answers2