char * function not displaying correctly in C

Question

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

char *czas()
{
  time_t rawtime;
  struct tm * timeinfo;
  char buffer [80];
  time ( &rawtime );
  timeinfo = localtime ( &rawtime );
  strftime (buffer,80,"Now it's %I:%M%p.",timeinfo);
  return buffer;
}

int main()
{
printf("%s",czas());
system("PAUSE");
}

I dont know why, but result of this program is only "Press any key(...)". I've tried also to print it as %c but it still doesn't work. What is wrong with this program?

Answer 1

Because buffer is a local variable that evaporates when the function returns - what you are seeing is undefined behaviour. A quick and dirty fix for this is to make the buffer static so that it hangs around after the function call - change:

char buffer [80];

to:

static char buffer [80];

Answer 2

The statement char buffer [80]; makes buffer allocated in csas's stack. Replace it with a call to malloc (char *buffer = malloc (80)) and you should be okay. You'll have to free the buffer yourself later.

Answer 3

You call czas() but after you return, the buffer that czas created no longer exists.

Answer 4

You are returning 'buffer' which is allocated on the stack. But once the function returns, that part of the stack is no longer valid.

Either allocate memory for the returned string on the heap or use std::string as the return value.

Answer 5

Your return buffer in the function should be somewhere other than on the stack. Since you have declared an auto variable as a fixed size array, it is on the stack. You return a pointer to it, BUT, that space can then be "scribbled over" by subsequent function calls.

Either:

• use a static buffer, and realize that the function is not re-entrant
• allocate a buffer with malloc(), and remember to free() it afterward in the caller

Both options have drawbacks. You will have 17 answers before I can enumerate them :-)

Answer 6

You are returning a pointer to a local variable ('buffer'), this is invalid and I'm surprised you don't receive a warning about it.

When a function exits all local variables cease to exist (known as going out of scope) and their memory will be used for other purposes. You are returning a pointer to this memory, but there's no guarantee what will now be there.

In this case it seems that at the time of the printf call the memory contains a 0 which is treated as an empty string. This is actually quite lucky, you could easily end up with either garbage being printed or the program crashing.

To resolve this you can either pass a buffer into czas, or have czas allocate a buffer on the heap which you later free. I would recommend the former because it is inline with how virtually all library functions work. It also avoids memory allocations where the caller must later free the pointer.

E.g:

size_t czas(char* buffer, size_t buffer_size)
{
    time_t rawtime;
    struct tm * timeinfo;

    time ( &rawtime );
    timeinfo = localtime ( &rawtime );
    return strftime (buffer, buffer_size,"Now it's %I:%M%p.",timeinfo);
}

int main()
{
    char buffer [80];
    if (czas(buffer, 80))
    {
      printf("%s\n",buffer);
    }
    else
    {
      printf("Call to czas failed");
    }
    system("PAUSE");
}

Update: I didn't notice that strftime took a size param, I've updated the code to use this and correctly pass back the result from strftime. As a result this is far more robust and you cannot accidentally overflow the buffer.

Answer 7

Don't use a static buffer unless you are sure you are never going to use this in multithreaded code, and that you'll never call it twice before using the first answer.

Malloc is an option, but forcing the caller to free the memory that a callee allocated can leave open issues of ownership and removes the possibility of using anything but heap memory for the buffer.

Your best bet, in my opinion, is to take a modification of Andrew Grant's suggest, but pass around the length of the buffer as well:

char *czas(char *buffer, size_t bufferLength)
{
  time_t rawtime;
  struct tm * timeinfo;
  time ( &rawtime );
  timeinfo = localtime ( &rawtime );
  strftime (buffer, bufferLength, "Now it's %I:%M%p.",timeinfo);
  return buffer;
}

int main()
{
   char buffer [80];
   printf("%s",czas(buffer, sizeof(buffer)));
   system("PAUSE");
}

Or

#define TIME_BUFFER_LENGTH 80
int main()
{
   char *buffer = malloc(TIME_BUFFER_LENGTH);
   if (buffer)
       printf("%s",czas(buffer, TIME_BUFFER_LENGTH));
   free(buffer);
   system("PAUSE");
}

This makes it easier to keep track of potential memory leaks, and buffer overflows. You can look at czas and see that as long at the arguments are correct, the function is not going to overflow any buffers, or leak any memory. Next, you can look at either version of main and see that no memory is leaked, and that the parameters passed to czas are correct (the bufferLength parameter accurately specifies the amount of space that buffer points to.)