functions returning char pointer

Question

I came across lot of functions returning char pointers in one legacy application. Some of them returning pointers to local character arrays. It seems to be causing crashes after several invocations(not immediately!) see the usage below

char *f1(){
  char buff[20];
  char *ptr;

  ----
  ----
  ptr=buff;
 return ptr;
}

---
---

f2(f1());

f1() returns a pointer local variable and then passes it to another function. I got the crash directly when it's compiled using _DEBUG mode in MS DEV. But in release mode, it doesnt cause an immediate crash, but it might occur after making lots of such calls.

When I modified the usage as below, it's working without any issues. Is the following usage safe?

strcpy(arr,f1()); /* arr is fixed char array*/
f2(arr);

Answer 1

No, it is undefined behaviour. It just happens to work in your case, but may stop working at any time.

Answer 2

No that's is not safe. Just calling strcpy can modify the stack enough to cause problems later because the return address and parameters might over-write the array.

Answer 3

The f1 function is returning a temporary (buff) which is freed when the function returns. You need to use malloc() inside the function.

Answer 4

No..its still not safe.

At the time you are doing strcpy(arr,f1());, the pointer used as the 2nd argument is already pointing to an array that does not exist.

Answer 5

Actually, the best would be to modify f1() to use malloc(). Your solution isn't near defined behavior.

Answer 6

Never return a pointer to local variable. It may work in some situations, but in general you will have lots of problems with it. Instead:

• document your function that it returns a pointer to allocated memory and that the returned buffer must be freed by the caller
• add a buffer and a size argument, and fill in the buffer (this is what is generally done in the Win32 API)
• if using C++, use std::string

Answer 7

Is the following usage safe?

No.

If your function returns a pointer to anything, make sure it allocates a memory area and returns the pointer to it:

char *safe_f1(void) {
    char *ptr;
    ptr = (char *) malloc(20 * sizeof(char));
    ...
    return ptr;
}

Answer 8

No, you see buff[20] is only available inside the f1 function. To be precise, the memory is allocated on f1s stack.

You need to create a new buff[20] on the heap using malloc and return a pointer to that memory from inside f1. Another way is to create buff[20] outside f1 (from the function which calls f1) and send it as an argument to f1. f1can then change the contents of the buffer and doesn't even have to return it.

Answer 9

the malloc solutions are interesting except that the memory should be free after usage. (outside the function). Else there would be a memory leak.

Answer 10

It is not safe. The reason is simple:

Any variable on a function will be allocated on the stack whose memory is freed after the function returns. The fact that the memory is freed does not mean that its content is changed.

This means that the memory content you did put in the variable char buff[20] is still at the buff or ptr (since ptr=buff) memory position. Whenever you call another function (or execute another block), its function/block variables will go to the stack too, creating the possibility of changing the memory content for whose the position ptr points to.

In the strcpy example you wrote, you were lucky enough that the strcpy function variables did not get on the stack at a position that were inside the old buff array. This is the reason you got the impression that it was safe.

The conclusion is that there is no way you can really guarantee that a freed memory content of the stack will not change between two function calls.

The solution is to use malloc because malloc does not allocate memory on the stack but on the heap. Heap memory is not deallocated unless you chose to do so (by a free call).

This approach would guarantee that the memory pointed by ptr is safe to be used by any other function.

The drawback of this solution is intrinsic: once the memory is not deallocated unless you programatically do that, if you forget to free this memory and lose the content of ptr, this memory will be there, allocated for your program but never achievable, lost for as long as your program runs. This memory will become a memory leak :-)

This is one reason why some languages have garbage collectors... but this is another story :-)

PS.: I think that it is safe (if your program is single threaded), although I do not recommend, to do something like that:

{
  char safe_buffer[20];
  char *unsafe_ptr;
  int i;
  unsafe_ptr = f1();
  /*Copy the buffer without calling any function
    not to change the stack content
  */
  for(i=0;i<20 && *(unsafe_ptr + i) != 0;i++)
  {
    *(safe_buffer + i) = *(unsafe_ptr + i);
  }
  *(safe_buffer + i) = 0;
  f2(safe_buffer);
}

Answer 11

I'd suggest two possible solutions:

1. Use a static char buff[20] in f1 unless the function is called from multiple threads or the outside world stores the pointer beyond the strcpy.

2. Use return strdup (ptr); and free the pointer outside f1. This is easier to use than malloc (though technically the same). It's slower than 1. but thread safe.

Answer 12

I would suggest changing these functions to take a pointer that it uses

void f1(char *)

That way every piece of code calling the function has to make a decision about where the memory gets written to, and to delete any memory that gets allocated.