C - What's wrong with my code (malloc, char*)

Question

I just want you to ask what did I do wrong with this code. I wrote a function that take a char* in parameter, I want to modify it directly without returning smthg, and reverse the string.

#include <iostream>

void reverseString(char *p_string){

    int length = strlen(p_string);
    int r_it = length - 1;
    char* tmp = (char*)malloc(length);
    int last_it = 0;

    for (int i = 0; i != length; i++){
        tmp[i] = p_string[r_it];
        r_it--;
        last_it++;
    }
    tmp[last_it] = '\0';

    strcpy_s(p_string, length + 1, tmp);
    //free(tmp);
}

int main(){

    char str[] = "StackOverflow";

    reverseString(str);

    std::cout << str << std::endl;

    system("pause");
}

I'm used to C++ and don't often use C functions like malloc/free/strcpy... Here, my problem is, when I alloc memory for my temporary char, I called mallec(length) for length = 13 in this case, char = 1 bytes so it should be allocate memory for 13 char is that right?

Problem is allocate more space than need so i need to use '\0' before my strcpy_s if not it breaks.

Did I do a mistake somewhere? Also, when i call free(tmp), it breaks too and say heap corruption, but I didn't free the memory before that.

Thanks for helping !

Answer 1

I took your original code and added a simple '+1' to the size of the malloc and got a passing result.

Not sure if your exercise is related specifically to the use of malloc, but have you considered doing the reversal directly inside the original string?

For example:

void reverseString(char *p_string){

    char* p_end = p_string+strlen(p_string)-1;
    char t;

    while (p_end > p_string)
    {
        t = *p_end;
        *p_end-- = *p_string;
        *p_string++ = t;
    }
}

int main(){

    char str[] = "StackOverflow";

    reverseString(str);

    std::cout << str << std::endl;

    system("pause");
}

If you are required to use malloc, then you need to ensure that you allocate enough space for string which includes the '\0'

Answer 2

You must use

int length = strlen(p_string);
int r_it = length - 1;
char* tmp = (char*)malloc(length+1);

Since strlen doesn't count the \0 character. So this will fail if you don't use length+1:

tmp[last_it] = '\0';

The length of a C string is determined by the terminating null-character: A C string is as long as the number of characters between the beginning of the string and the terminating null character (without including the terminating null character itself). http://www.cplusplus.com/reference/cstring/strlen/

Btw. C99 support semi dynamic arrays. So could you try this:

char tmp[length+1];

Source: http://en.wikipedia.org/wiki/Variable-length_array

float read_and_process(int n)
{
    float vals[n];

    for (int i = 0; i < n; i++)
        vals[i] = read_val();
    return process(vals, n);
}

Answer 3

Check the below C code: The memory allocated to tmp should be length+1 as done below and also there are many unnecessary variables which can be avoided.

#include<stdio.h>
#include<string.h>
#include<stdlib.h>
void reverseString(char *p_string){

   int i;
   int length = strlen(p_string);
   int r_it = length - 1;
   char* tmp = (char*)malloc(length+1);

   for (i = 0; i != length; i++){
      tmp[i] = p_string[r_it--];
   }   
   tmp[i] = '\0';

   strcpy(p_string, tmp);
   return;

}

int main(){

   char str[] = "StackOverflow";

   reverseString(str);

   printf("%s",str);
   return 0;

}

Answer 4

There is nothing fundamentally wrong with your approach, just some of the details. Since I am not sure how you found out that the sizeof(tmp) is 32, I modified your code to the one below which includes a few printfs and some minor changes:

#include "stdio.h"
#include "stdlib.h"
#include "string.h"

void reverseString(char *p_string)
{
  size_t length = strlen(p_string);
  size_t r_it = length - 1;
  char* tmp = (char*)malloc(length+1);
  int last_it = 0;
  size_t i=0;

  printf("strlen(p_string) = %d\n", strlen(p_string));
  printf("Before: strlen(tmp) = %d\n", strlen(tmp));

  for (i = 0; i != length; i++) {
    tmp[i] = p_string[r_it];
    r_it--;
    last_it++;
  }

  tmp[last_it] = '\0';

  printf("After: strlen(tmp) = %d\n", strlen(tmp));

  strcpy(p_string, tmp);

  free(tmp);
}

int main()
{
  char str[] = "StackOverflow";

  reverseString(str);

  printf("%s\n", str);

  return 0;
}

First, I have removed all C++ specific code - you can now compile this with gcc. Running this code yields this output:

sizeof(p_string) = 13
Before: strlen(tmp) = 0
After: strlen(tmp) = 13
wolfrevOkcatS

This is to be expected - strlen basically counts bytes until it hits the \0 character and so the first time we print the size using strlen, it returns 0 since we just allocated the memory. As another poster suggested, we have to allocate 1 extra byte to store the \0 in our new string.

Once the reverse is complete, 13 bytes would have been copied over to this memory and the second strlen returns the expected answer.