Returning a string to a variable length character array

No, it isn't safe to return a pointer to a local string in a function. C won't stop you doing it (though sometimes the compiler will warn you if you ask it to; in this case, the local variable return_string prevents it giving the warning unless you change the code to return filled_buffer;). But it is not safe. Basically, the space gets reused by other functions, and so they merrily trample on what was once a neatly formatted string.

Can you explain this comment in more detail — "No, it isn't safe..."

The local variables (as opposed to string constants) go out of scope when the function returns. Returning a pointer to an out-of-scope variable is undefined behaviour, which is something to be avoided at all costs. When you invoke undefined behaviour, anything can happen — including the program appearing to work — and there are no grounds for complaint, even if the program reformats your hard drive. Further, it is not guaranteed that the same thing will happen on different machines, or even with different versions of the same compiler on your current machine.

Either pass the output buffer to the function, or have the function use malloc() to allocate memory which can be returned to and freed by the calling function.

Pass output buffer to function

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

int reverse_string(char *input_string, char *buffer, size_t bufsiz);

int reverse_string(char *input_string, char *buffer, size_t bufsiz)
{
    size_t j = 0;
    size_t i = strlen(input_string);
    if (i >= bufsiz)
        return -1;

    buffer[i] = '\0';
    while (i != 0)
    {
        buffer[j] = input_string[i-1];
        i--;
        j++;
    }
    printf("%s\n", buffer);
    return 0;
}

int main (void) 
{
    char buffer[16];
    if (reverse_string("tasdflkj", buffer, sizeof(buffer)) == 0)
        printf("%s\n", buffer);
    return 0;
}

Memory allocation

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

char *reverse_string(char *input_string);

char *reverse_string(char *input_string)
{
    size_t j = 0;
    size_t i = strlen(input_string) + 1;
    char *string = malloc(i);
    if (string != 0)
    {
        string[--i] = '\0';
        while (i != 0)
        {
            string[j] = input_string[i-1];
            i--;
            j++;
        }
        printf("%s\n", string);
    }
    return string;
}

int main (void) 
{
    char *buffer = reverse_string("tasdflkj");
    if (buffer != 0)
    {
        printf("%s\n", buffer);
        free(buffer);
    }
    return 0;
}

Note that the sample code includes a newline at the end of each format string; it makes it easier to tell where the ends of the strings are.

This is an alternative main() which shows that the allocated memory returned is OK even after multiple calls to the reverse_string() function (which was modified to take a const char * instead of a plain char * argument, but was otherwise unchanged).

int main (void) 
{
    const char *strings[4] =
    {
        "tasdflkj",
        "amanaplanacanalpanama",
        "tajikistan",
        "ablewasiereisawelba",
    };
    char *reverse[4];
    for (int i = 0; i < 4; i++)
    {
        reverse[i] = reverse_string(strings[i]);
        if (reverse[i] != 0)
            printf("[%s] reversed [%s]\n", strings[i], reverse[i]);
    }
    for (int i = 0; i < 4; i++)
    {
        printf("Still valid: %s\n", reverse[i]);
        free(reverse[i]);
    }
    return 0;
}

Also (as pwny pointed out in his answer before I added this note to mine), you need to make sure your string is null terminated. It still isn't safe to return a pointer to the local string, even though you might not immediately spot the problem with your sample code. This accounts for the garbage at the end of your output.

First, returning a pointer to a local like that isn't safe. The idiom is to receive a pointer to a large enough buffer as a parameter to the function and fill it with the result.

The garbage is probably because you're not null-terminating your result string. Make sure you append '\0' at the end.

EDIT: This is one way you could write your function using idiomatic C.

//buffer must be >= string_length + 1
void reverse_string(char *input_string, char* buffer, size_t string_length)
{
    int i = string_length;
    int j = 0;

    while (i != 0)
    {
        buffer[j] = input_string[i-1];
        i--;
        j++;
    }
    buffer[j] = '\0'; //null-terminate the string

    printf("%s", buffer);
}

Then, you call it somewhat like:

#define MAX_LENGTH 16

int main()
{
    char* foo = "foo";
    size_t length = strlen(foo);
    char buffer[MAX_LENGTH];

    if(length < MAX_LENGTH)
    {
        reverse_string(foo, buffer, length);

        printf("%s", buffer);
    }
    else
    {
        printf("Error, string to reverse is too long");
    }
}