Reverse a string using pointers in a function, output in main is garbled

I'm trying to self-study C using C Primer Plus from Stephen Prata and one of the end-of-chapter exercises is to "Write a function that replaces the contents of a string with the string reversed.". This is a chapter on character strings with a good dose of pointers. I'm trying to use pointers as much as possi开发者_如何学运维ble so I can better understand, but I'm stuck.

My problem is that when I print the value of the return pointer in main, it is garbled.

When I use gdb(just learning how to use that too), I can see that the memory address returned from my function is the same address that was used in the function and it's getting assigned to my pointer in main okay as far as I can tell.

I've tried so many things, what am I missing? FWIW I have not learned about malloc yet in the book, though I see it referenced on various www pages I've frequented trying to better understand C.

$ cc -o exercise8 exercise8.c && ./exercise8
This is s1 before: abcd
This is s2 in function: dcba
This is s3 after: d`!

/*   A function that replaces the contents of a string with the string reversed. */
#include <stdio.h>
#include <string.h>
char *str_rev(char * string);
int main(void)
{
   char * s1  = "abcd";
   char * s3;
   printf("This is s1 before: %s\n", s1);

   s3 = str_rev(s1);
   printf("This is s3 after: %s\n", s3);

}

char *str_rev(char * string)
{
   char ar3[5];
   char * s2;
   int len = 0;

   s2 = ar3;

   len = (strlen(string) - 1);
   string = string + len;

   while ( len >= 0 )
   {
      *s2 = *string;
      len--;
      string--;
      s2++;
   }
   s2++;
   *s2 = 0;
   s2 = s2 - 5;

   printf("This is s2 in function: %s\n", s2);
   return s2;
}

$ gdb exercise8
GNU gdb (GDB) 7.1-ubuntu

Reading symbols from exercise8...done.
(gdb) break 12
Breakpoint 1 at 0x804844a: file exercise8.c, line 12.
(gdb) break 40
Breakpoint 2 at 0x80484d9: file exercise8.c, line 40.
(gdb) run
Starting program: exercise8
This is s1 before: abcd         // My original string.
This is s2 in function: dcba        // Good, my reversed string while in the function.

Breakpoint 2, str_rev (string=0xbffff043 "dcba") at exercise8.c:40
40               return s2;
(gdb) print s2
$1 = 0xbffff043 "dcba"          // Location of pointer s2.
(gdb) continue
Continuing.

Breakpoint 1, main () at exercise8.c:12
12               printf("This is s3 after: %s\n", s3);
(gdb) print s3
$2 = 0xbffff043 "dcba"          // Back in main same pointer as s2 from function.
(gdb) step
This is s3 after: d`Q           // Line 12 executed.  Output garbled.
14      }
(gdb)

You're returning a pointer to a local variable, (automatic variable to speak in ISO standard terms,) which is allocated on the stack, as soon as you return from your function that memory is released leaving you with a dangling pointer pointing to memory that may or may not still contain the string you put there, it depends entirely on circumstances. You should provide the output buffer as a function argument, or allocate it with malloc, or in C++ with new.

edit; added some example code

void reverse(const char* s1, char* s2) {
  const int l = strlen(s1);
  const char* p = s1 + l - 1;
  do {
    *s2++ = *p;
  } while (p-- != s1);
  *s2 = 0;
}

int main() {
  // some code here
  char s1[5] = "abcd";
  char s2[5] = "";

  reverse(s1, s2);

  // some more code here

  return 0;
}

or

char* reverse(const char* s) {
  const int l = strlen(s);
  char* rs = malloc(l+1);
  const char* p = s + l - 1;
  do {
    *rs++ = *p;
  } while (p-- != s);
  *rs = 0;
  return rs - l;
}

int main() {
  // some code here
  char s1[5] = "abcd";

  char* s2 = reverse(s1);

  // some more code here

  free(s2);

  return 0;
}

char ar3[5];
char * s2 = ar3;

The above code will make s2 points to a character string on the stack. This ar3 variable will be deleted once your function finishes.

You should output to some variable that you have pre-allocated. Modify it as follow

int main(void)
{
   char * s1  = "abcd";
   char s3[5];
   printf("This is s1 before: %s\n", s1);

   str_rev(s1, s3);
   printf("This is s3 after: %s\n", s3);

}

void str_rev(char * string, char * s2)
{
   ........

   // don't return

   // Also assign the last character with the NULL terminator
   ar2[strlen(string)] = '\0';
}

Of course, once you get to the chapter regarding malloc, you can allocate the necessary memory for s3 depending on the length of s1. Until then, read on and have fun.

The problem description sounds like you can just reverse the string in place. Keep it simple.

void reverse_range(char *first, char *last) // [first, last)
{
    for (; first != last && first != --last; ++first)
    {
        char temp = *first; *first = *last; *last = temp;
    }
}

void reverse(char *str)
{
    reverse_range(str, str + strlen(str));
}

int main()
{
    char text[] = "0123456789";

    printf("before: %s\n", text);
    reverse(text);
    printf("after : %s\n", text);
}

s2 points to your local ar3 array. Therefore when str_rev returns, it is no longer valid to look at the memory where ar3 was via s2. Now s2 is called a dangling pointer, and this is one of the huge pains in learning to use C pointers correctly.

For a simple solution that doesn't use malloc and meets the exercise requirement of "replaces the contents of a string", try copying the result into the function argument pointer (the original string; but be careful since your current code has since changed the pointer string). You know this pointer points at memory with enough characters and isn't local to your function.

Part of the problem is that you can't actually "replace the contents of a string with the string reversed" when you are dealing with string literals, ie. strings delcared in the form char * s1 = "abcd";

Without using literals, I made a relatively easy to understand recursive example:

/*   A function that replaces the contents of a string with the string reversed. */
#include <stdio.h>
#include <string.h>
void str_rev(char * string);

int main(void)
{

   char s1[] = "abc";
   char s2[] = "even";
   char s3[] = "quodd";


   printf("This is s1 before: %s\n", s1);
   str_rev(s1);
   printf("This is s1 after: %s\n", s1);

   printf("This is s2 before: %s\n", s2);
   str_rev(s2);
   printf("This is s2 after: %s\n", s2);

   printf("This is s3 before: %s\n", s3);
   str_rev(s3);
   printf("This is s3 after: %s\n", s3);

    return 0;

}

void str_rev(char * string) {

    //Store the first char of the string locally
    char firstChar = string[0];
    //Store the last char of the string locally
    int lastCharPos = strlen(string)-1;
    char lastChar = string[lastCharPos];

    //Shorten the string (temporarily)
    string[lastCharPos] = '\0';


    if (string[1] != '\0') {
        //Call on the now shortened string, eg.
        //"abc" becomes "b"
        //"even" becomes "ve"
        //"quodd" becomes "uod"
        str_rev(string+1);
    }

    //Swap the first and last characters
    string[0] = lastChar;
    string[lastCharPos] = firstChar;

}

On my system the output is as follows:

This is s1 before: abc
This is s1 after: cba
This is s2 before: even
This is s2 after: neve
This is s3 before: quodd
This is s3 after: ddouq

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

void my_strrev(char* begin){
    char temp;
    char* end;
    end = begin + strlen(begin)-1;

    while(end>begin){
        temp = *end;
        *end = *begin;
        *begin = temp;
        end--;
        begin++;
    } 
}

main(){
    char string[]= "foobar";
    my_strrev(string);
    printf("%s", string);
}