What are the issues with this code for reversing a string in memory?

[James889]

Hi,

I'm trying to write a simple program for reversing a string stored in memory.

This is what I've come up with. For some reason this code coredumps, so something is wrong.
any ideas ?

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

char *reverse(char *buf,unsigned int bufsize ){

        /* Allocate memory for the reversed string */
        char *ptr = (char *)malloc(bufsize);

        /* Get the address of the last character in the buffer. */
        int buffaddr = &buf[bufsize];
        char *buffptr = (char *)buffaddr;

               *ptr++ = *buffptr--;
return ptr;
}


int main(void){

        char Buffer[100];

        strcpy(Buffer,"a cup of coffee");

        char *reversed = reverse(Buffer,sizeof(Buffer));

free(*reversed);

return 0;
}

 

[jedishrfu]

Don't you need a while loop in the reverse function to iterate over the string?

Also you could have simply added the bufsize to your buffptr to get the end of the buffer.

The real problem is that your string lengths are variable and you need to start copying at the end of the string not the end of the buffer.

The core dump is due to your buffptr pointing past the end of the buffer. Your buffer goes from 0 to 99 but you referenced 100.

 

[Medicol]

Still, your program won't work.
For example, dynamic memory allocation with new operator (for explanation purpose). I don't know how to explain this with malloc (my teacher started the pointer section since early last week ).

char* a=new char;
char* a=new char[10];

they only differ in array of chars being allocated. In your program, this similar "a" pointer is initialized to nothing and therefore

*a++=*b--;

won't copy anything into a. But if you do this

a[i]=*b--;i++;

it will work. Because "a" then knows where it is going to assign the value to.
And finally your while loop may look like this

while (*buffptr)
	{
		ptr[i] = *buffptr--;	
		i++;
	}

Don't forget to assign the last character bit of this string to '\0'. And your good question is very tricky.

 

[rcgldr]

I'm trying to write a simple program for reversing a string stored in memory.

That problem statement seems to imply that the goal is to reverse a string in place as opposed to making a reversed copy of that string. This can be done via indexing or via pointers.

 

[James889]

One thing i don't understand.
The statement 'char **bufptr = buf' and putchar(bufptr) whould print out the same output as putchar(buf). But it doesn't. Why?

char *reverse(char *buf){

        /* Allocate memory for the reversed string */
        char *ptr = (char *)malloc(100);

        char **bufptr = buf;

        /* This should return the same output as putchar(buf), but doesn't. Why? */
        putchar(bufptr);
        }

 

[chiro]

With pointers, a char** is a pointer to a pointer of a character array while char* is a pointer to the starting offset of a character array.

If you are returning the pointer to the offset of a character array then you are returning a char* (which should be valid, allocated, and point to the right thing).

The typical reason for using double pointers (like char**) is that you pass it so that you can modify a variable in the scope of a function and have it stay that way after the function terminates. Outside of this kind of application, it makes no sense to use double pointers for most purposes.

Also you need to return a pointer to some character array and your function doesn't seem to do this.

 

[James889]

So why do i get different results from *buf and **bufptr ?

 

[rcgldr]

    char *reversed = reverse(Buffer,sizeof(Buffer));
    free(*reversed);

The core dump probably occurs on the free. It should be free(reversed), without the *, but another issue is that reverse() allocates memory in ptr, but returns an incremented copy of ptr, so that will also cause free() to fail.

char *reverse(char *buf){
        char *ptr = (char *)malloc(100);
        char **bufptr = buf;

You could set bufptr to point to the address the local copy (on the stack) of buf. Somewhat useless but it would work:

char *reverse(char *buf){
        char *ptr = (char *)malloc(100);
        char **bufptr = &buf;
/* ... after this, then (*bufptr)[i] refers to the same location as buf[i]; */

Assuming this isn't homework, then here is an alternative:

void reverse(char * buf, int bufsize)
{
int mid = bufsize/2;
int i;
char c;
    for(i = 0; i < mid; i++){
        c = buf[i];
        buf[i] = buf[bufsize-i-1];
        buf[bufsize-i-1] = c;
    }
}

 

[Mark44]

With pointers, a char** is a pointer to a pointer of a character array

No, you're off in the levels of indirection. char** is a pointer to a pointer to a character.

while char* is a pointer to the starting offset of a character array.

Strictly speaking, char* is a pointer to a char. That character might or might not be in an array of characters. If the character happens to be in an array, it could be any character in the array, not necessarily the beginning character.

If you are returning the pointer to the offset of a character array then you are returning a char* (which should be valid, allocated, and point to the right thing).

The typical reason for using double pointers (like char**) is that you pass it so that you can modify a variable in the scope of a function and have it stay that way after the function terminates. Outside of this kind of application, it makes no sense to use double pointers for most purposes.

Also you need to return a pointer to some character array and your function doesn't seem to do this.

 

[voko]

Multiple problems with the originally posted code.

char *reverse(char *buf,unsigned int bufsize )

What is the meaning of the bufsize variable? The length of the original buffer (1), or the length of the original string (2)? Or the length of the buffer to allocate (3)? Of all these, only (2) would be really useful to reverse(). (1) gives to reverse() insufficient information, so it will still have to find out the length of the original string. (3) is even worse, because the length of the string still needs to be found, and it may so happen that it is greater than the size of the buffer to allocate - what is reverse() to do in such a situation?

        int buffaddr = &buf[bufsize];

Regardless of the (1), (2) & (3) options above, this is simply wrong, because (A) the address obtained by the right-hand side is not of the int type; and (B) the address of the buf[bufsize] in cases (1) & (3) points just after the end of the buffer; in case (2), it points to the terminating zero character, which may or may not be wrong. (A) can easily result in a bogus address being eventually assigned to buffptr in the next line. In short, do not do it: there is simply no reason to convert the address to int in this case. To address problem (B), you need to obtain the true length of the string.

               *ptr++ = *buffptr--;

This line is meaningless. In case (2), it will copy just the terminating zero character; in cases (1) and (3) it will, at best, copy some random garbage character; or it can dereference an invalid address location and make your program go boom.

return ptr;

Bad, too, because it returns not the address of the allocated buffer, but the address somewhere (hopefully) in this buffer.

free(*reversed);

*reversed is a character, whose numeric value is between 0 and 255, inclusively. This is then converted to an address, which free() expects. This address will be within the first 256 bytes of the process's address space. On most modern operating systems, such an address is invalid.

What you want to do is free(reversed) - assuming you deal with all the other problems.

 

[James889]

Multiple problems with the originally posted code.
What is the meaning of the bufsize variable? The length of the original buffer (1), or the length of the original string (2)? Or the length of the buffer to allocate (3)? Of all these, only (2) would be really useful to reverse(). (1) gives to reverse() insufficient information, so it will still have to find out the length of the original string. (3) is even worse, because the length of the string still needs to be found, and it may so happen that it is greater than the size of the buffer to allocate - what is reverse() to do in such a situation?
Regardless of the (1), (2) & (3) options above, this is simply wrong, because (A) the address obtained by the right-hand side is not of the int type; and (B) the address of the buf[bufsize] in cases (1) & (3) points just after the end of the buffer; in case (2), it points to the terminating zero character, which may or may not be wrong. (A) can easily result in a bogus address being eventually assigned to buffptr in the next line. In short, do not do it: there is simply no reason to convert the address to int in this case. To address problem (B), you need to obtain the true length of the string.
This line is meaningless. In case (2), it will copy just the terminating zero character; in cases (1) and (3) it will, at best, copy some random garbage character; or it can dereference an invalid address location and make your program go boom.
Bad, too, because it returns not the address of the allocated buffer, but the address somewhere (hopefully) in this buffer.
*reversed is a character, whose numeric value is between 0 and 255, inclusively. This is then converted to an address, which free() expects. This address will be within the first 256 bytes of the process's address space. On most modern operating systems, such an address is invalid.

What you want to do is free(reversed) - assuming you deal with all the other problems.

void reverse(char *buf){

unsigned int strlen = 0;
  /* Allocate memory for the reversed string */
  char *ptr;
  ptr = (char *)malloc(100);
 

/* Find the length of the string in the buffer */
  while( *buf++ != '\0'){
  strlen++;
  }
/* Copy */
for(strlen;strlen>0;strlen--){
  *ptr++ = *buf--;
}}

Could you do something similar to this?

 

[voko]

  ptr = (char *)malloc(100);

Bad. This must be the real size required for the string, not some constant.

/* Find the length of the string in the buffer */
  while( *buf++ != '\0'){
  strlen++;
  }

OK, but why not use strlen()?

/* Copy */
for(strlen;strlen>0;strlen--){
  *ptr++ = *buf--;
}

Bad.

1. When the loop starts, buf points just after the end of the original string, not even at its zero terminator.

2. There is no code that will zero-terminate the new string.

3. And I do not see that the new string is returned.

 

[rcgldr]

If you want to returned a reversed copy of the input string, try this:

char * reverse(char * buf)
{
int buflen = 1;
char * src;
char * dst;
char * rtn;
    if(!buf)
        return(0);
    src = buf;
    while(*src++)
        buflen++;
    dst = rtn = malloc(buflen);
    src--;
    while(src != buf)
        *dst++ = *--src;
    *dst = 0;
    return(rtn);
}

 

[Mark44]

If you want to returned a reversed copy of the input string, try this:

char * reverse(char * buf)
{
int buflen = 1;
char * src;
char * dst;
char * rtn;
    if(!buf)
        return(0);
    src = buf;
    while(*src++)
        buflen++;
    dst = rtn = malloc(buflen);
    src--;
    while(src != buf)
        *dst++ = *--src;
    *dst = 0;
    return(rtn);
}

Based on what's in the OP...

I'm trying to write a simple program for reversing a string stored in memory.

... I don't believe that the goal is to return a reversed copy of the input string, but instead to actually reverse the input string. The usual way of doing this is to use a loop that swaps the first and last nonnull character, then swaps the second and the next to last, and so on until you get to the middle of the string. You definitely don't want to be swapping characters after you get to the middle, or you undo all the previous swaps.

 

[rcgldr]

Based on what's in the OP...
... I don't believe that the goal is to return a reversed copy of the input string, but instead to actually reverse the input string.

Which I already posted an example of back in post #8, but the thread kept going.

 

[James889]

Ok, so after a bit of fiddling i finally managed to come up with a version that works(not saying anything about code quality etc ;) )

char *reverse(char *buf){

  /* Allocate enough memory for the string */
  int length = strlen(buf);
  char *ptr = calloc(length,sizeof(char));
  char **p2p;
  p2p = ptr;

  /* Null terminate the string */
  *(ptr+length) = '\0';

  for(int i=length-1;i>=0;i--){

  *( ptr + i ) = *buf++;
  }  return p2p;

}

 

[Mark44]

Ok, so after a bit of fiddling i finally managed to come up with a version that works(not saying anything about code quality etc ;) )

char *reverse(char *buf){

  /* Allocate enough memory for the string */
  int length = strlen(buf);
  char *ptr = calloc(length,sizeof(char));
  char **p2p;
  p2p = ptr;

  /* Null terminate the string */
  *(ptr+length) = '\0';

  for(int i=length-1;i>=0;i--){

  *( ptr + i ) = *buf++;
  }  return p2p;

}

The main problems I see are the following.
1. p2p is declared as type char ** (i.e., pointer to a pointer to a char). In the line following the declaration, you set p2p to ptr. Since ptr is of type char * and p2p is of type char **, you have different levels of indirection. Your compiler should at least have generated a warning about this.
2. ptr is initialized with the value returned by calloc(), which is the address of a block of length bytes. This memory can be treated as if it were an array, with indexes running from 0 through length - 1. A few lines later you put a null character at index length. IOW, you are setting memory that doesn't belong to the array, which is not a good thing to do.

In the OP you said this:

I'm trying to write a simple program for reversing a string stored in memory.

Your buf parameter contains the address of a string already stored in memory. Why don't you reverse that string rather than make a reversed copy of the string?