Why might string concatenation prefix each concatenation with garbage?

Question

I'm pretty new to C, so I apologize if this is pretty standard knowledge..

I have a function like so, where I am appending a bunch of C-style strings together and outputting it:

char *example(int n, int days, int years){
    char *ret;
    if (n < 5) {
        ret = (char*)malloc(sizeof(char)*256);
        sprintf(ret, "There are %d days in %d years", days, years);
        ret = (char*)malloc(strlen(ret));
        return ret;
    }
    else {
        char *s1;
        char *s2;
        char *s3;
        s1 = example(n/2, days, years);
        s2 = example(n + 5, days, years);
        s3 = example(n--, days, years);

        int length = strlen(s1) + strlen(s2) + strlen(s3);
        ret = (char*)malloc(length);
        strcat(ret, s1);
        strcat(ret, s2);
        strcat(ret, s3);

        return ret;
       }
}

This is prefixing each of the new concatenations with a few garbage characters. I'm assuming my issue is in my memory management, but I'm not sure.. Is this simple? What have I done wrong? Also, how can this be done cleaner?

Answer 1

Why did you write that call to reallocate the string?

ret = (char*)malloc(strlen(ret));

How do you suppose that the characters you put into that string in the line above it would make it over to the newly-allocated memory?

Take that second call to malloc out and see if that helps. Also, when you allocate the string in the "else" clause, you need to make room for the terminating null character, so add 1 to your "length". Right after the malloc call, set *ret to 0.

Answer 2

You allocate a chunk of memory, write into it with sprintf, then throw away the first buffer and replace it with a new buffer of undefined contents.

    ret = (char*)malloc(sizeof(char)*256);
    sprintf(ret, "There are %d days in %d years", days, years);
    ret = (char*)malloc(strlen(ret));
    return ret;

Deleting the third line will likely help.

Answer 3

I didn't use c for a very long time but...
In case the n < 5 you have the following code:

ret = (char*)malloc(sizeof(char)*256);
sprintf(ret, "There are %d days in %d years", days, years);
ret = (char*)malloc(strlen(ret));

why do you need the last line?

Answer 4

strlen() returns the number of characters NOT including the terminating zero ('\0'). When allocating memory you should add 1 to strlen() to hold that zero char.

You should put that '\0' manually at the beginning of your string or use strcpy() instead of the first strcat().

Answer 5

This

ret = (char*)malloc(sizeof(char)*256);
sprintf(ret, "There are %d days in %d years", days, years);
ret = (char*)malloc(strlen(ret));

is allocating some memory, writing a string into that memory, and then allocating and returning new, uninitialised memory.

This:

ret = (char*)malloc(length);
strcat(ret, s1);

is appending s1 to ret, which is uninitialised. You need to do

ret[0] = '\0'

after allocating the memory to initialise it as a zero-length string.

Answer 6

char *example(int n, int days, int years){
   char *ret;
   if (n < 5) {
       ret = (char*)malloc(sizeof(char)*256);
       sprintf(ret, "There are %d days in %d years", days, years);
//     ret = (char*)malloc(strlen(ret));  (DELETE THIS LINE)
       return ret;
   }
   else {
       char *s1;
       char *s2;
       char *s3;
       s1 = example(n/2, days, years);
       s2 = example(n + 5, days, years);  // WILL THIS CAUSE INFINITE RECURSION?
       s3 = example(n--, days, years);

       int length = strlen(s1) + strlen(s2) + strlen(s3) + 1; // ALLOW ROOM FOR TERMINATING '\0'
       ret = (char*)malloc(length);
       strcpy(ret, s1);  // CHANGE TO strcpy()
       strcat(ret, s2);
       strcat(ret, s3);

       return ret;
   }
}