C reverse string function getting weird output

Question

I'm trying to understand pointers and made a reverse string function.
code:

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

char *reverseString(char string[]){
    int i;
    int len = strlen(string);
    char reversedString[len];  
    for (i = 0; i < len; i++){
        reversedString[i] = string[(len - 1) - i];
    }
    printf("%s", reversedString); //print it out

    return reversedString; //return pointer to first element of reversed string
}

int main (){
    char string[6] = "kitten";
    int i;
    char *p = reverseString(string);
    return (0);

}

My goal is to reverse the string "kitten" and print the reversed string. I expect the output "nettik" but I get "nettik��". Why am I getting these weird characters?

Once the strlen() problem is fixed, stackoverflow.com/questions/25603908/… is another reverse-string question with your other bug (returning a local array). — Paul Roub
– Paul Roub, Commented Aug 19, 2015 at 21:10
@pascx64 Are you saying that returning the local reversedString variable is good, safe, well-defined behavior? — Paul Roub
– Paul Roub, Commented Aug 19, 2015 at 21:11
No, this is why is said "even if it is a bug". It is just, not THE bug that cause the problem of gilianzz — pascx64
– pascx64, Commented Aug 19, 2015 at 21:13
I think the equality of "weird output" and "undefined behavior" should be documented somewhere. — Eugene Sh.
– Eugene Sh., Commented Aug 19, 2015 at 21:14

lynn · Accepted Answer · 2015-08-19 21:14:15Z

Stop! First of all, you're making a classic mistake beginners often make when they learn about pointers. When you write:

char *reverseString(char string[]) {
    ...
    char reversedString[len];
    return reversedString;
}

You're returning a pointer to where it was, not is: the memory actually gets freed when you leave the function, so there's no guarantee that the memory you think contains your reversed string isn't already getting reused by something else. This way, your program will fail catastrophically.

However, you're printing the reversed string before returning it, so it's no huge problem yet. What's going wrong there, then, is that your string isn't properly zero-terminated.

A string in the C language needs room to store its final '\0' byte: char string[6] = "kitten"; is too small to hold the zero-terminated string { 'k', 'i', 't', 't', 'e', 'n', '\0' }. Similarly, when you call printf("%s", reversedString), you haven't properly terminated the string with '\0', so printf keeps looking for the end of the string and prints out whatever junk is in memory where reversedString is allocated.

You should try:

making your function return void,
allocating 7 bytes for your original string,
allocating strlen(string) + 1 (which will also be 7) bytes for your new string, and
writing a '\0' to the end of your new string after looping through the old one in reverse.

juanchopanza · Accepted Answer · 2015-08-19 21:18:18Z

9

There are at least two major bugs in your code. There could be more.

First, the call to strlen causes undefined behaviour because you aren't passing it a null-terminated string. The reason for this is that your char array isn't large enough for the null terminator character:

char string[6] = "kitten";

You need

char string[7] = "kitten";

or

char string[] = "kitten";

Second, you are returning a pointer to a local array, namely reversedString. De-referencing that would also cause undefined behaviour. You can solve this by wither reversing the string in-place, or passing a pointer to a buffer of the same length as the input. Remember that the reversed string must also be null-terminated.

edited Aug 19, 2015 at 21:18

answered Aug 19, 2015 at 21:10

juanchopanza

228k35 gold badges421 silver badges493 bronze badges

5 Comments

Jonathan Leffler Over a year ago

Interestingly, the code doesn't dereference the returned pointer-to-local-variable, so that isn't an active problem (but it is a disaster waiting to happen). Not null-terminating the string in the reverse function (as well as not passing a null-terminated string to a function that expects one) is a problem. The printing occurs down in the reverse function where the local variable is valid.

juanchopanza Over a year ago

@JonathanLeffler Yes, it is a potential problem. If the function is used as expected, UB will ensue even if the null-terminations are fixed. I re-phrased my answer to indicate that it could be a problem.

chux Over a year ago

@Jonathan Leffler I think even copying an invalid pointer is UB. Certainly de-referencing it is bad.

Jonathan Leffler Over a year ago

@chux: In theory, yes. In practice, I've not heard of a machine where the assignment in main() would cause problems. Any use of the result could cause problems; possibly even checking the result for nullness could cause problems. But the systems where that happens are few and far between. I'm not excusing the returned pointer; it is bad. I am suggesting that the lack of null terminators, and lack of space in the VLA for a null terminator, are the main problems.

chux Over a year ago

@Jonathan Leffler I used to think the same thing about signed integer overflow, but then the compilers became "smarter". I agree with current practice, copying an invalid pointer is is not a great concern but a "disaster waiting to happen".

Vlad from Moscow · Accepted Answer · 2015-08-19 21:56:58Z

The function is wrong.

Firts of all it returns pointer to a local array that will be destroyed after exiting the function

//,,,
char reversedString[len];  
//...
return reversedString; //return pointer to first element of reversed string

Secondly the reversed string shall have terminating zero. However you declare an array that has no space for the terminating zero.

Also this array in main defined incorrectly

char string[6] = "kitten";

because it does not include terminating zero.

And at last it is a bad design of the function.

If you want to copy the source string in the destination string then the both character arrays should be declared as function parameters. Moreover the source array should be declared as a constant array.

The function can look the following way

char *reverseCopyString( char s1[], const char s2[] )
{
    size_t n = strlen( s2 );

    for ( size_t i = 0; i < n; i++ ) s1[i] = s2[n - i - 1];
    s1[n] = '\0';

    return s1;
}

Or you could define the function such a way that ir reverse the source string. For example

char *reverseString( char s[] )
{
    size_t n = strlen( s );

    for ( size_t i = 0; i < n / 2; i++ )
    {
        char c = s[i];
        s[i] = s[n - i - 1];
        s[n - i - 1] = c;
    }        

    return s;
}

Take into account that string literals are immutable in C and may not be changed. Any attempt to change a string literal results in undefined behaviour of the program.

Here is a demonstrative program

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

char *reverseCopyString( char s1[], const char s2[] )
{
    size_t n = strlen( s2 );

    for ( size_t i = 0; i < n; i++ ) s1[i] = s2[n - i - 1];
    s1[n] = '\0';

    return s1;
}

char *reverseString( char s[] )
{
    size_t n = strlen( s );

    for ( size_t i = 0; i < n / 2; i++ )
    {
        char c = s[i];
        s[i] = s[n - i - 1];
        s[n - i - 1] = c;
    }        

    return s;
}

int main( void )
{
    char *s1 = "Hello gilianzz";
    char s2[16];

    puts( s1 );
    puts( reverseCopyString( s2, s1 ) );
    puts( reverseString( s2 ) );
}

The program output is

Hello gilianzz
zznailig olleH
Hello gilianzz

Collectives™ on Stack Overflow

C reverse string function getting weird output

3 Answers 3

Comments

5 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

5 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related