1

Defined in h file:

char *copiedText;

Defined in c file:

char *copyText(char *text){
  if (copiedText != 0) free(copiedText);
  copiedText = (char *)calloc((strlen(text) + 1) * sizeof(char), sizeof(char));
  strcpy(copiedText, text);
  return copiedText;
}

First off, this isn't about how to copy text, I just chose that as an example. My question is about the free() before calloc().

First question I hear you ask is why not just free() at the appropriate time - i.e. when copiedText is no longer needed?

Long story short, I'm making part of a program and I can not trust the users of my function to properly free() copiedText, so I want to contain as much code inside my function as possible. All they do is include h and c file and call the function copyText.

I am aware there will be a (minor) memory leak between last call to copyText and program termination, but this is a tradeoff that I am willing to accept, because I only malloc() small amounts of data.

The question is simply, will the free() actually free up the memory allocated by my calloc() when coded like this?

Woodgnome
  • 2,281
  • 5
  • 28
  • 52
  • I think the answer from aix below is spot on. I'd like to add that if you can't trust your users to do proper memory management in C, they really shouldn't be programming in C. It's really not possible or practical to idiot proof C. That's why we have Java :-) – Dave Rager Nov 10 '11 at 15:20
  • Never declare a variable in a .h file. It's a recipe for disaster. Putting an extern declaration is no problem, but things that generate code and objects (i.e. things that will appear in an .o file) should never be put in a header. – Patrick Schlüter Nov 10 '11 at 16:11
  • What exactly is the issue in not declaring the variable as extern in the .h file? – Woodgnome Nov 10 '11 at 17:33
  • @Woodgnome, see [this question](http://stackoverflow.com/q/2649427/416627) for that. – Dave Rager Nov 10 '11 at 18:57

5 Answers5

3

The flip side is that now:

  1. the pointer returned by copyText() is only valid until the next call to copyText() -- this is not at all obvious to the caller and is probably worth documenting as prominently as you can;
  2. the function is not thread-safe.

A better approach might be to require an explicit free() (perhaps wrapping it in a function called freeText(), as a counterpart to copyText()).

Another alternative is for the caller to provide their own buffer along with its size (so that you can avoid buffer overruns).

NPE
  • 486,780
  • 108
  • 951
  • 1,012
  • I take that as an "Yeah you can free() and then malloc(), but..." Cheers for pointing out the thread issues and ideas for a better solution. Luckily it shouldn't be a problem though, because each call to my function will be made sequentially. – Woodgnome Nov 10 '11 at 15:52
1

What's wrong with strdup()? The calloc() seems useless because you immediately overwrite the memory with what's in copiedText also. You're effectively "double-initializing" which is just a waste of time.

As far as the free() goes, I'm not sure as to what you're referring to? The memory will be free'd, however, it may or may not be released to the system due to memory fragmentation. I really wouldn't worry about that though...

dlamotte
  • 6,145
  • 4
  • 31
  • 40
1

If you can't trust your users to free(), I certainly wouldn't trust them with this function. Without view of the code, it's not obvious that every call to copyText() obliterates the call from the last.

 main(){
    char *a=copyText("Hello, ");
    char *b=copyText("World!\n");
    printf("%s%s",a, b);
}

Certainly doesn't work as expected.

In threaded environments, the effects of this are even more pronounced, and dangerous. It's entirely possible that one thread frees copiedText as another is writing to it, leading to undefined behavior. It's even conceivable that one thread shrinks copiedText to the size of its parameter, then another thread writes a large amount of data, thinking it had sufficient space, and overruns the buffer, making your code vulnerable to heap overflow attacks.

Dave
  • 10,964
  • 3
  • 32
  • 54
  • Luckily it's all sequential and old pointers will not be used after a new call to my function. – Woodgnome Nov 10 '11 at 15:53
  • If you can **absolutely guarantee** that, then this would work. You might consider removing `copiedText` from the header, (returning the pointer makes that superfluous) and instead making it a `static` variable in your function. – Dave Nov 10 '11 at 15:57
  • The reason I put it in the .h file was to make sure it would be defined for subsequent calls - can `static` achieve this as well? – Woodgnome Nov 10 '11 at 17:56
  • `static` would achieve that, but putting it in the `.h` file is misleading. That creates a *separate* variable of the same name in every file which includes the header. Again, in this specific usage -- always including the c and h files, compiling just the top level file -- it works, but you're eschewing standard practices again, which will cause confusion down the road. – Dave Nov 10 '11 at 19:03
  • And we have to stick with standard practice of course ^_^ Side question though, say I put this in my .h file `#ifndef __HEADER_H_INCLUDED__ #define __HEADER_H_INCLUDED__ [code here] #endif` would it still be an issue? – Woodgnome Nov 11 '11 at 08:15
  • Yes. The correct thing to do would be to have `char *copyText(char *);` in the H file, then have the implementation in a c file, which you would compile to a `.o` file. Any files which include the H file would then be *linked* against the `.o` – Dave Nov 11 '11 at 09:27
  • I think I get the differences between static and extern variables now - and why static would be the better choice here. – Woodgnome Nov 11 '11 at 10:52
1

The use of a global variable is the bad idea here. It is a hidden side effect. It seems easy to follow now, but in 6 month when your project will have 10000 loc you will forget about it. So don't do it. As a rule, avoid side effects, avoid function that do multiple different unrelated things, avoid functions that are to long.

char *copyText(const char *text)
{
   return strcpy(malloc((strlen(text) + 1)), text);
}
Patrick Schlüter
  • 11,394
  • 1
  • 43
  • 48
0

i think a simple 

char *copyText(char *text)
{
 if (copiedText != NULL)
   free(copiedText);
copiedText = strdup(text);
return copiedText;
}

should work for what you want, but like aix said all the pointers from previous call will no longer exist.

Bit-Doctor
  • 53
  • 6