-1
 1| typedef struct container{
 2|     char* abc;
 3| }container;
 4|
 5|
 6| int main(void){
 7|
 8|     container* xyz = malloc(sizeof(container));
 9| 
10|     xyz->abc = malloc(10);
11|     xyz->abc = "abcdefghi\0";
12| 
13|     free(xyz->abc);
14|     free(xyz);
15| }

According to Valgrind there's a leak on line 10. which means free(xyz->abc) isn't working. How can I free this?

Sourav Ghosh
  • 133,132
  • 16
  • 183
  • 261
Aditya Pokharel
  • 161
  • 1
  • 2
  • 8

4 Answers4

7

You allocated 10 bytes, and stored a pointer to those bytes (xyz->abc) then you immediately replaced that pointer with a pointer to a string literal ("abcdefghi\0").

There are several problems with your program:

  1. Ten leaked bytes
  2. Excess \0 in string literal — why?
  3. free-ing something you didn't malloc (the replaced pointer)
  4. Using malloc/free in the first place (assuming the tag isn't spurious)

In C, use strdup to emplace abcdefghi into your dynamically-allocated buffer.

In C++, forget this and switch to std::string.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • Don't use `strncpy`. From your linked man page: "Notes. Some programmers consider strncpy() to be inefficient and error prone." [Why are strlcpy and strlcat considered insecure?](http://stackoverflow.com/questions/2114896/why-are-strlcpy-and-strlcat-considered-insecure). – Lundin May 03 '17 at 14:30
  • @Lundin: What a load of nonsense. – Lightness Races in Orbit May 03 '17 at 14:35
  • 1
    Umm no. Among veteran C programmers, `strncpy` is widely known to be dangerous, for the reason that people always use it incorrectly and screw up null termination. It was never intended as a bounds-checking version of `strcpy`, we nowadays have `strcpy_s` for that purpose (introduced with C11). There's a lot of brainwashed dogma around blindly replacing `strcpy` with `strncpy`. What one should do instead is of course to check the bounds before using `strcpy`, or alternatively use `memcpy` or `strcpy_s`. – Lundin May 03 '17 at 14:39
  • @Lundin: We will just have to agree to (very, very strongly) disagree. It would also be nice if you would present your arguments without doing so in the form of insinuating (a) I'm not a veteran, just because I disagree with you, or (b) I'm brainwashed, or (c) I follow "dogma". Thanks. – Lightness Races in Orbit May 03 '17 at 14:40
  • 1
    If you wanted a respectful comment, you should probably not reply with "What a load of nonsense" with no counter arguments. Here's some arguments www.google.com then type `strncpy is dangerous`. The 5-10 first hits all look quite relevant. As for brainwashing, Microsoft has done a lot of that with VS false positive warning for the use of `strcpy`. – Lundin May 03 '17 at 14:43
  • 1
    [Why is strncpy insecure?](http://stackoverflow.com/questions/869883/why-is-strncpy-insecure) – Lundin May 03 '17 at 14:45
  • 1
    As for veteran C programmers, they have already gone through all of these phases: 1) lets use `strcpy` 2) lets not use `strcpy`, random Microsoft dummies say its dangerous! Use `strncpy`. 3) Hmm `strncpy` kind of sucks I think. It is really hard to get the null termination right. 4) `strncpy` definitely sucks, why is this? Oh wow it wasn't intended to be used in the way I've been using it for years. Turns out it's a 1970s remain from UNIX that got included in the standard C library mostly by accident. 5) lets use `strcpy`. – Lundin May 03 '17 at 14:51
  • Personally, I advocate the use of `strdup`, which would s available on all the platforms which interest me and is easily implemented if necessary. `xyz->abc = strdup("abcdedghi");` would have avoided all problems. – rici May 03 '17 at 15:36
  • @rici: Much better - I'm going to steal that for my answer; thanks! – Lightness Races in Orbit May 03 '17 at 15:37
3

In your code, xyz->abc finally does not hold a pointer returned by memory allocator function, it holds the pointer to the first element of the array containing the string literal "abcdefghi\0". You don't need to free() it.

Rather, passing xyz->abc to free() in current scenario causes undefined behavior, so the answer boils down to, you must not attempt to pass a pointer to free() which is not returned by earlier malloc() and family or NULL.

Quoting C11, chapter §7.22.3.3

[....] Otherwise, if the argument does not match a pointer earlier returned by a memory management function, or if the space has been deallocated by a call to free or realloc, the behavior is undefined.

Sourav Ghosh
  • 133,132
  • 16
  • 183
  • 261
1

Put strcpy( xyz->abc, "abcdefghi" ); at line 11.

With xyz->abc = "abcdefghi\0"; you redirect xyz->abc pointer to static string and lose the allocated memory address. Then free will work with wrong address and will not free the dynamically allocated block. \0 is not necessary as mentioned above by @BoundaryImposition - every double quote enclosed string is null terminated and you add extra 0.

i486
  • 6,491
  • 4
  • 24
  • 41
  • No, this will cause UB by modifying a string literal. Further modifications are necessary. – Lightness Races in Orbit May 03 '17 at 14:29
  • Furthermore, `strcpy` is **dangerous** and should be avoided at all times. – Lightness Races in Orbit May 03 '17 at 14:29
  • @BoundaryImposition I agree, but `malloc(10)` (constant) is another wrong decision. With `strcpy` the exact example will work. – i486 May 03 '17 at 14:30
  • @BoundaryImposition What is UB? why `strcpy` is dangerous? – RoiHatam May 03 '17 at 14:31
  • 2
    @BoundaryImposition Unlike `strncpy`, [strcpy() is perfectly safe](http://stackoverflow.com/a/23490019/584518). – Lundin May 03 '17 at 14:34
  • @RoiHatam It is not dangerous, that's a wide-spread, incorrect myth. `strncpy` is however intrinsically dangerous and should never be used for any purpose. – Lundin May 03 '17 at 14:35
  • Since `container.abc` is never used (in given code snippet) as a null-terminated string, it's perfectly valid to use `strncpy`. But it's essential to remember that `strncpy` should *never* be used with null-terminated strings. – Sergej Christoforov May 03 '17 at 14:54
  • 1
    This question is looking for an *explanation*, not simply for working code. Your answer provides no insight for the questioner, and may be deleted. Please [edit] to explain what causes the observed symptoms. – Toby Speight May 03 '17 at 14:58
  • @TobySpeight The questioner can think what is the difference. Otherwise there is no good perspective to become programmer. – i486 May 03 '17 at 18:15
0

The value passed to free() must be a value returned by something like malloc() or realloc(). Any other value passed to free() is invalid.

Your code allocates memory using malloc() and assigns the result to xyz->abc. But the next line then assigns the address of "abcdefghi\0" to xyz->abc.

So at this point, xyz->abc no longer holds the value returned by malloc().

And when you pass this value to free(), that is not valid, and no memory is freed. That is your leak.

Jonathan Wood
  • 65,341
  • 71
  • 269
  • 466