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);
}

"it's working fine, but I keep getting memory trash when I print the result" I do not get that. Either - or. Not both. — Yunnosch, Nov 01 '22 at 15:48
First you pass `strlen(string_test) - 1` as the string length to your function, which isn't the string length. Then you use `len - 1` in your loop, which might or might not be wrong. But worse is that your `str_copy` array don't have enough space for the null-terminator. So it's a good thing (well, not really) that you don't actually add the null-terminator. And you use the index `len`, which will be out of bounds. Plenty of chances for *undefined behavior*. — Some programmer dude, Nov 01 '22 at 15:51
Your `str_copy` is too small. You might want to use your debugger to watch the variables. — the busybee, Nov 01 '22 at 15:51
For this case, you could use `strncpy()` instead of `strcpy()` to copy the unterminated character data back to the source string. — John Bollinger, Nov 01 '22 at 15:58
Also, you remove the newline from the input (but not in a [good way](https://stackoverflow.com/questions/2693776/removing-trailing-newline-character-from-fgets-input)) then add a newline in the string you create (even though the original string you pass in doesn't have a newline). Why? — Some programmer dude, Nov 01 '22 at 15:58

Craig Estey · Answer 1 · 2022-11-01T23:49:11.413

A few issues ...

Issues in main:

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

Issues in reverse_string:

Terminates string with \n instead of 0x00 -- this is what causes "trash characters" to appear at the end.
No need to copy the string -- the reverse can be done "in place"
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;
    }
}

@RuudHelderman There were so many issues that I missed this. Code edited. — Craig Estey, Nov 01 '22 at 23:50

Keep getting memory trash in my string in C

1 Answers1