Keep getting memory trash in my string in C

Question

I made a program that reverses a string in C, it's working fine, but I keep getting memory trash when I print the result. I tried this:

That's my program:

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

#define SIZE_STRING 20

void reverse_string(char* string, int len);

int main() {
    char string_test[SIZE_STRING];
    printf("What's the string? ");
    fgets(string_test, SIZE_STRING, stdin);
    string_test[strlen(string_test) - 1] = '\n';
    reverse_string(string_test, strlen(string_test) - 1);
    printf("The new string is: %s", string_test);

   return 0;
}

void reverse_string(char* string, int len){
    char str_copy[len];
    int i, j;
    for(i = 0, j = len - 1; i < len; i++, j--)
    {
        str_copy[i] = string[j];
    }
    str_copy[len] = '\n';
    strcpy(string, str_copy);
}

Answer 1

A few issues ...

Issues in main:

1. Does not strip a newline
2. Passes incorrect length to reverse_string (i.e. both decrement length by 1)
3. Assumes returned string has a newline
4. Does not handle immediate EOF or line without a newline (from a "degenerate" input file)

Issues in reverse_string:

1. Terminates string with \n instead of 0x00 -- this is what causes "trash characters" to appear at the end.
2. No need to copy the string -- the reverse can be done "in place"
3. Instead of i < len, using i < j is a bit better. With the former it is doing extra work (i.e. double the number of loops).

---

Here is the refactored code. It is annotated with the bugs and fixes:

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

// NOTE/BUG: size is a bit small -- with larger size better chance to _not_
// truncate the newline
#if 0
#define SIZE_STRING 20
#else
#define SIZE_STRING 1000
#endif

void reverse_string(char *string, int len);

int
main(void)
{
    char string_test[SIZE_STRING];

    printf("What's the string? ");
#if 0
    fgets(string_test, SIZE_STRING, stdin);
#else
    if (fgets(string_test, SIZE_STRING, stdin) == NULL)
        string_test[0] = 0;
#endif

// NOTE/BUG: does _not_ strip newline
// NOTE/BUG: passing incorrect length to reverse_string (because it passes
// len - 1 but reverse_string does this also)
#if 0
    string_test[strlen(string_test) - 1] = '\n';
    reverse_string(string_test, strlen(string_test) - 1);
#else
    size_t len = strlen(string_test);

    // strip newline
    if ((len > 0) && (string_test[len - 1] == '\n')) {
        string_test[len - 1] = 0;
        --len;
    }

    reverse_string(string_test, len);
#endif

// NOTE/FIX: reverse_string will no longer append newline
#if 0
    printf("The new string is: %s", string_test);
#else
    printf("The new string is: %s\n", string_test);
#endif

    return 0;
}

void
reverse_string(char *string, int len)
{
// NOTE/BUG: no need to make copy -- we can do the reverse in-place
#if 0
    char str_copy[len];
#endif
    int i, j;

#if 0
    for (i = 0, j = len - 1; i < len; i++, j--) {
        str_copy[i] = string[j];
    }
#else
    for (i = 0, j = len - 1; i < j; i++, j--) {
        char tmp = string[i];
        string[i] = string[j];
        string[j] = tmp;
    }
#endif

// NOTE/BUG: we want to terminate the string with 0x00 and _not_ newline
// NOTE/FIX: with in-place we already have a 0x00
#if 0
    str_copy[len] = '\n';
#endif

#if 0
    strcpy(string, str_copy);
#endif
}

---

In the above code, I've used cpp conditionals to denote old vs. new code:

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

#define SIZE_STRING 1000

void reverse_string(char *string, int len);

int
main(void)
{
    char string_test[SIZE_STRING];

    printf("What's the string? ");
    if (fgets(string_test, SIZE_STRING, stdin) == NULL)
        string_test[0] = 0;

    size_t len = strlen(string_test);

    // strip newline
    if ((len > 0) && (string_test[len - 1] == '\n')) {
        string_test[len - 1] = 0;
        --len;
    }

    reverse_string(string_test, len);

    printf("The new string is: %s\n", string_test);

    return 0;
}

void
reverse_string(char *string, int len)
{
    int i, j;

    for (i = 0, j = len - 1; i < j; i++, j--) {
        char tmp = string[i];
        string[i] = string[j];
        string[j] = tmp;
    }
}