Trouble solving Invalid write of size 1 please advice

I wrote a reverse function it works fine but having trouble solving valgrind throwing

Code:

---

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

char *strrev(char *str){
  char *tmp = malloc(sizeof(char) * strlen(str));
  memset(tmp,0,strlen(str));
  for(int i=0, k=(int)strlen(str); i<=(int)strlen(str); i++, k--){
    printf("%c\n", str[k]);
    strncat(tmp, &str[k], 1);
  }
  return tmp;
}
int main(){
    char *str = strrev("ABCD");
    printf("%s\n", str);
    free(str);
    return 0;
}

---

Please advice

Eh. You need to account for the terminating null character. You only need to call strlen(str) once: store the result in a size_t variable. Your loop index variables should be size_t too. Finally, strncat is overkill and inefficient: just assign characters.

Another approach could be to implement an in-place string reverse, then call that in-place string reverse to implement this string copy and reverse.

By the way, I suggest you rename your strrev function: if I remember correctly, it is technically a reserved name (something to with the str prefix, I think, rather than there being such a standard function), and more practically it could conflict with a strrev provided as a standard library extension.

yes thank you ! darn I missed the terminating 0/

Code:

---

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

char *strrev(char *str){
  size_t len = strlen(str);
  char *tmp = malloc(sizeof(char) * len + 1);
  memset(tmp, 0, len);
  for(size_t i=0, k=len-1; i<len; i++, k--){
      tmp[i] = str[k];
  }
  tmp[len] = 0;
  return tmp;
}
int main(){
    char *str = strrev("ABCD");
    printf("%s\n", str);
    free(str);
    return 0;
}

---

All works fine now !

Not that your strrev() is wrong, but I prefer to follow C's standard library philosophy and assume the arguments are correct and test/initialize them outside the funcion. I would implement strrev() this way:

Code:

---

// Notice: No malloc() and just one call to strlen().
char *strrev2( char *destp, const char *srcp )
{
  const char *p;
  char *q;

  // Points to source string NUL char.
  p = srcp + strlen( srcp );

  // Initially the destination string is empty.
  *destp = 0;

  // if source string isn't empty...
  if ( *p-- )
  {
    // Copy bytes in reverse order.
    q = destp;
    while ( p >= srcp )
      *q++ = *p--;

    // puts the final NUL char at destination string.
    *q = 0;
  }   

  // Returns destination string pointer.
  return destp;
}

---

If you pass invalid pointers you'll get a 'segmentation fault' or misbehavior the same way it happens to strlen() (passing a NULL pointer to srcp, for example).

And you'll need to allocate the destination string before calling the function, the same way it happens with strcpy(), for example.

Reverse in-place using swaps:

Code:

---

#include <string.h>

char* reverse(char* text)
{
 char* left = text;
 char* right = text + strlen(text);
 while(left < --right)
 {
  char saved = *left;
  *left++ = *right;
  *right = saved;
 }
 return text;
}

char* reversed(char* destination, const char* source, size_t max)
{
 strncpy(destination, source, max);
 return reverse(destination);
}

#include <stdio.h>

int main(void)
{
 char buffer[1024];
 for(;;)
 {
  printf("Input to reverse (press `enter` to exit) > ");
  fgets(buffer, sizeof(buffer), stdin);
  char* newline = strstr(buffer, "\n");
  if(newline)
  *newline = 0;
  if(*buffer == 0)
  break;
  puts(reverse(buffer));
 }
}

---

Note that the null-termination step isn't necessary here since it's only the enclosing chars are being swapped out.

Quote:

---

Originally Posted by Sir Galahad

Reverse in-place using swaps

---

Nice! ;)

Thanks. And kudos to you for a better job of code documentation. I do try to make mine as self-documenting as possible, but in the process oft times forget to do much commenting...