2

I'm (attempting?) returning memory allocated inside a function.

Assuming there are no obvious problems with what I wrote here (and didn't test) how is the memory freed or not freed? I need to free(bob) later because he's on the heap not the stack, right?

I read something about reference counting in C in another accepted answer just now but I really really don't remember C having anything like a garbage collector.

char* strCat5000(char *fmt, ...)
{
  char buff[5000];
  char *ret_;
  va_list arg_ptr;
  va_start(arg_ptr, fmt);
  vsnprintf(buff, sizeof(buff), fmt, arg_ptr);
  va_end(arg_ptr);
  //ret_ = malloc((char*)(strlen(buff)+1)*sizeof(char)); //Allocated inside function
  ret_ = (char*)malloc((strlen(buff)+1)*sizeof(char)); //moved (char*) .. typo
  ret_ = strcpy(ret_,buff);
  return (ret_);
}
...
void findBob()
{
  char * bob;
  bob = strCat1000("Server is %s for %d seconds.", "on fire", 35329);
  printf("%s", bob);
  free(bob); //bob needs to be freed here or he'll leak when findBob ends???
}
Community
  • 1
  • 1
AppFzx
  • 1,445
  • 2
  • 14
  • 23
  • 2
    reference counting has nothign to do with garbage collection. It is a pattern used to manage memory, but it's still manual from the programmers POV – Ed S. Oct 12 '13 at 18:34
  • You're right, C doesn't have a garbage collector. Is there a problem with your code? – Barmar Oct 12 '13 at 18:35
  • @EdS.: Not necessarily. Python uses reference counting, and it's transparent to the programmer. – icktoofay Oct 12 '13 at 18:35
  • 1
    @icktoofay That's because Python is also doing the `malloc`s itself. – Barmar Oct 12 '13 at 18:36
  • 2
    You need to free the bob. Nothing is automatic. – Arpit Oct 12 '13 at 18:36
  • @Barmar I don't know if there's a problem. That's part of the problem. I might have a leak and I might be free-ing when I don't have to. (curse having to program in .NET for money... I've forgotten how to write a real program.) – AppFzx Oct 12 '13 at 18:37
  • @icktoofay: Likewise with CEF (Chromium Embeddable) and I believe also Mozilla Firefox, if I remember correctly. It can be done very transparently, maybe not with "pure" C but at least with C++. – Per Lundberg Oct 12 '13 at 18:37
  • @Barmar: Yes, but it's still garbage collection. – icktoofay Oct 12 '13 at 18:37
  • Does this bit even compile "malloc((char*)(strlen(buff)+1)*sizeof(char));". Besides why write such incomprehensible code – Ed Heal Oct 12 '13 at 18:39
  • I note that you're casting the argument to `malloc` to a `char *`. That's very strange; typically, you'd only cast the *return value*, and [then it's still somewhat discouraged](http://stackoverflow.com/q/605845). Secondly, `sizeof(char)` is always 1, although feel free to be explicit I guess. – icktoofay Oct 12 '13 at 18:39
  • @EdHeal It shouldn't compile. I'll fix that. (char*) should have been outside. – AppFzx Oct 12 '13 at 18:40
  • @AppFzx - You do not even need it outside either – Ed Heal Oct 12 '13 at 18:43
  • @AppFzx - And you do not need the `sizeof(char)` bit as well – Ed Heal Oct 12 '13 at 18:44
  • @icktoofay: Python uses reference couting to *implement* a GC – Ed S. Oct 12 '13 at 18:48
  • Hacked the question a bit so we don't do a lot of manual `malloc/strcpy` when strdup exists (at least in POSIX). – Per Lundberg Oct 12 '13 at 18:51
  • @PerLundberg Better not to modify the question and remove parts which have discussion existing. I'm modifying your modification to preserve original as comments. – AppFzx Oct 12 '13 at 18:54
  • @PerLundberg nevermind.. someone already rejected it – AppFzx Oct 12 '13 at 18:56
  • @AppFzx: possibly. Just couldn't resist one chance of doing some nice refactorings. ;) – Per Lundberg Oct 12 '13 at 18:57

4 Answers4

1

Yes. You must "free Bob" in the findBob() method, since otherwise there will be no further notion of that particular piece of memory in your program => you will have a leak. So, in short: yes, that memory must be freed.

But then, just a side note: this kind of code is very "error prone". If you have a function allocating memory which the caller is supposed to free, it might be a good idea to have a freeBob() method or similar, which makes this purpose very obvious. It's usually a good idea to have malloc() and free() calls happen at the same level of abstraction. E.g. two functions in the same .c file might be a reasonable "abstraction level" in this case.

Per Lundberg
  • 3,837
  • 1
  • 36
  • 46
  • I quite completely disagree with the idea that free-ing inside a 3rd function to preserve *level of abstraction* makes code more readable or maintainable but... from hunting around the interwebs, that does look like what's proper. – AppFzx Oct 12 '13 at 19:05
  • Well, if you think of a public API (which this may or may not be), it feels more natural to have a `get_foo()` method which returns some form of data structure, and a `free_foo()` method coupled along with it to take care of the deallocation. Yes, it might feel like "meaningless extra code", but I think it's good. What if `get_foo` would allocate an object, which has an embedded extra-object which has to be deallocated also, for example? So, I actually think that this approach is quite good; it makes the code more robust in my eyes. – Per Lundberg Oct 12 '13 at 19:11
1

You're correct, if you return heap-allocated data from a function, it needs to be freed eventually (unless you need it until the program is finished, then you can depend on the fact that all memory is freed when the process exits).

Reference counting is a technique used to avoid freeing memory too soon. If you may copy a pointer into a number of variables, you don't want to free it while any of them are still using it. You allocate a structure that contains a refcount member. Every time you assign the pointer to another variable, you must increment the refcount, as well as decrement the refcount of whatever that variable was pointing to previously. When the refcount drops to 0, you can call free().

Wikipedia

Barmar
  • 741,623
  • 53
  • 500
  • 612
  • "then you can depend on the fact that all memory is freed when the process exits". True, but this kind of coding is generally considered a bit sloppy I would say... – Per Lundberg Oct 12 '13 at 18:46
  • this part: "you may depend on the fact that all memory is freed when the process exits" gives me the creeps. Is that documented somewhere outside wikipedia to work cross platform with all (reputable) compilers? – AppFzx Oct 12 '13 at 18:46
  • @AppFzx: I don't think there would be an operating system in production use where this does not apply. Still, I agree 100%, it's bad practice to write code that works like that. It's much better to have an assertion that runs when the program exits to make sure that all (global) state has been properly deallocated, or something like that (and then of course also perform that deallocation. ;) – Per Lundberg Oct 12 '13 at 18:47
  • It's part of the definition of a process. – Barmar Oct 12 '13 at 18:47
1

To handle the above issue you have two approaches.

1st is what you are doing, that is free the bob after its use.Because nothing is automatic in c.

2nd is you can pass parameter to strCat1000 in which you want to store the result(let say it as store_var). In that case store_var will be on stack (it must be declared inside the findBob) and it will be automatically deallocated after findBob ends.

If you want freedom from free then make your code free from malloc.

Something like this could work:

void findBob()
{
  char store_var[1000];
  strCat1000(store_var,"Server is %s for %d seconds.", "on fire", 35329);
  ...

void strCat1000(char to_save[],char *fmt, ...)
{
  ret_ = strcpy(to_save,buff); //check for lengths also.
  ...
Per Lundberg
  • 3,837
  • 1
  • 36
  • 46
Arpit
  • 12,767
  • 3
  • 27
  • 40
0

To make it real simple I wrote the following peace of code (c++). just run it and monitor your code memory usage using task manager.

#include "stdafx.h"
#include<iostream>
#include <conio.h>

using namespace std;
int * allocate(unsigned size)
{
    int * arr = new int[size];
    for(unsigned i=0;i<size;i++)
        arr[i] = i;
    return arr;
}
int main()
{
    int * p;

    p = allocate(1000000);
    getch(); // Here you still occupy the memory
    free(p); // here you free the memory you occupy in the function
    getch(); // here you should see that memory is freed before returning from the main
    return 0;
}
AndyBaba
  • 875
  • 5
  • 7
  • This looks reasonable but I'm arbitrarily confining myself to C for this project because I decided I'm offended by C++'s numerous problems. In fact I'm migrating to http://software.intel.com/en-us/intel-cilk-plus as soon as I have time to breath. – AppFzx Oct 12 '13 at 19:08