Getting Segmentation fault in my program at string concatenation

Question

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

char *GetTodayDate()
{
   char *buffer;
   time_t t = time(NULL);
   struct tm tm = *localtime(&t);
   char monBuffer[3] ;
   char dayBuffer[3];
   char yearBuffer[5];

   sprintf(monBuffer, "%02d", tm.tm_mon + 1);
   sprintf(dayBuffer, "%02d", tm.tm_mday);
   sprintf(yearBuffer, "%d",  tm.tm_year + 1900);


   strcat(buffer, monBuffer);
   strcat(buffer, "/");
   strcat(buffer, dayBuffer);
   strcat(buffer, "/");
   strcat(buffer, yearBuffer);

   return buffer;
}

int main()
{
     char *today;
     sprintf(today,"%s",GetTodayDate());
     // I want to print the today string here to check, please give me proper statement here
     return 0;
}

I am getting segmentation fault in my program, I want to store dayBuffer, monBuffer, yearBuffer buffers into a single buffer, so that I can pass this buffer to another program. I want to return this buffer to another program

Answer 1

As others have pointed out, you must allocate memory. The question then becomes "who should allocate the memory". Allow me to propose three fairly standard ways of answering this question with their drawbacks and advantages.

One possibility would be to have callers allocate memory: (I also added error reporting because that's one of the reasons why I like this method better than the two others, feel free to ignore it).

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

#define DATE_SIZE 12
enum error_codes {OK = 0, ERROR = -1};
// You could have multiple error codes

int GetTodayDate(char *buffer);

int main()
{
     char *today = malloc(DATE_SIZE);
     int err = GetTodayDate(today);
     if(err == OK)
         printf("Today is %s\n", today);
     else
         printf("Error getting date\n")
     free(today);
     return 0;
}

int GetTodayDate(char *buffer)
{
   /* Get the info */
   time_t t = time(NULL);
   struct tm tm = *localtime(&t);

   /* Convert it to strings */
   char monBuffer[3] ;
   char dayBuffer[3];
   char yearBuffer[5];
   sprintf(monBuffer, "%02d", tm.tm_mon + 1);
   sprintf(dayBuffer, "%02d", tm.tm_mday);
   sprintf(yearBuffer, "%d",  tm.tm_year + 1900);

   if( something_went_wrong_here)
        return -1;

   /* Put in the buffer */
   *buffer = '\0'; // same as doing buffer[0] = '\0'; but is more C-stylish
   strcat(buffer, monBuffer);
   strcat(buffer, "/");
   strcat(buffer, dayBuffer);
   strcat(buffer, "/");
   strcat(buffer, yearBuffer);

   return 0; // To indicate success, 
}

Then another option, which is NOT THREAD SAFE but is used by many C standard library functions is that will allow you to call your function to return a string without worrying about memory management is the following: It uses a static array of chars in the function GetTodayDate. This makes it so that the array, although it acts as a local variable, continues to exist after the function returns (a static local variable is actually a global variable that is only accessible within the function).

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

#define DATE_SIZE 12

char *GetTodayDate();

int main()
{
     // Easier to use but has drawbacks
     printf("Today is %s", GetTodayDate());
     return 0;
}

char *GetTodayDate()
{
   /* Get the info */
   time_t t = time(NULL);
   struct tm tm = *localtime(&t);

   /* Convert it to strings */
   char monBuffer[3] ;
   char dayBuffer[3];
   char yearBuffer[5];
   sprintf(monBuffer, "%02d", tm.tm_mon + 1);
   sprintf(dayBuffer, "%02d", tm.tm_mday);
   sprintf(yearBuffer, "%d",  tm.tm_year + 1900);

   /* Put in the buffer */
   static char buffer[DATE_SIZE];
   buffer[0] = '\0';
   strcat(buffer, monBuffer);
   strcat(buffer, "/");
   strcat(buffer, dayBuffer);
   strcat(buffer, "/");
   strcat(buffer, yearBuffer);
   return buffer;
}

This also has the drawback that if you want to use that value later, you have to copy it using strcpy(). But you can see that using the function is much more simple.

And the third option is to have GetTodayDate allocate the memory, which has the advantage that it knows how much memory to allocate, thus liberating the user from having to know that detail. It has the disadvantage that since the client didn't allocate the memory, he might have more chances of forgetting to free it.

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

#define DATE_SIZE 12

int GetTodayDate(char *buffer);

int main()
{
     char *today = GetTodayDate();
     printf("Today is %s", today);
     // Easier to forget to do if 
     // we're not the one who wrote GetTodayDate
     free(today); 
     return 0;
}

char *GetTodayDate()
{
   /* Get the info */
   time_t t = time(NULL);
   struct tm tm = *localtime(&t);

   /* Convert it to strings */
   char monBuffer[3] ;
   char dayBuffer[3];
   char yearBuffer[5];
   sprintf(monBuffer, "%02d", tm.tm_mon + 1);
   sprintf(dayBuffer, "%02d", tm.tm_mday);
   sprintf(yearBuffer, "%d",  tm.tm_year + 1900);

   /* Put in the buffer */
   char *buffer = malloc(DATE_SIZE);
   buffer[0] = '\0';
   strcat(buffer, monBuffer);
   strcat(buffer, "/");
   strcat(buffer, dayBuffer);
   strcat(buffer, "/");
   strcat(buffer, yearBuffer);

   return buffer;
}

I personally prefer the first one because it allows me to do error reporting with the return value. Also, a lot of C standard library functions work that way : call a function and tell it (via a parameter) where to put the info you requested. This is especially useful when you want a function to return more than one thing without copying.

struct BigStruct bs;
// get me this info and here's 
// where you're going to put said info.
if(get_BigStruct(&bs)){
     printf("could not get info\n");
     return ERROR;
}
printf("field1 %d\n", bs.field1);

But I digress, the point is that I like the first one best because 1) Error reporting 2) Allocation and deallocation are done at the same place 3) Common C way of doing "get me this info and here's where you're going to put it".

Answer 2

You're not even allocating buffer so that's undefined behaviour, writing in an unallocated/random area.

char *buffer;

in this case, could be safely replaced by

char *buffer = malloc(11); buffer[0]= '\0';

which is the size you need to hold a YYYY/MM/DD date, and initialized to 0 so the first strcat works properly (another case of undefined behaviour otherwise)

I would do it in one sprintf call, though, without all the temp buffers and the strcat calls:

char *buffer = malloc(11); /* no need to initialize it beforehand */
sprintf(buffer, "%02d/%02d/%d", tm.tm_mon + 1, tm.tm_mday, tm.tm_year + 1900);

(since you're returning buffer, you cannot use char buffer[11]; even if it's tempting since scope is limited to the current routine)

Aside: same issue in main(). You're writing (again) in undefined memory. Since you allocated the memory just do:

int main()
{
     char *today = GetTodayDate();
     printf("The date is %s\n",today);
     free(today); // free memory, even if it's not really necessary here, you're quitting the program
     return 0;
}

Answer 3

strcat(buffer, monBuffer);

You haven't allocated any memory for buffer, thus you get segfault because buffer does not point to any writable memory block, allocated from heap, to hold your data. Allocate some memory with malloc for your string. And don't forget to return that memory to heap with free like this:

char *GetTodayDate()
{
    char *buffer = malloc (MAX_DT_STRING_LEN);
    /* ... */

    *buffer = 0;

then in main():

char *today = GetTodayDate();

printf ("%s", today);
free (today);

It is good practice to initialize pointers and to check values returned from malloc and other routines, which may allocate memory, against NULL.

You may also preallocate a buffer and pass a pointer to it to your GetTodayDate().

Here MAX_DT_STRING_LEN is the length of your buffer. Choose arbitrary reasonable length for the buffer to be able to hold maximally possible date/time string.