-1

I am currently writing a program to simply reverse a string in C. However, when I try to copy the contents of the temp string I made into the original string, I get a segmentation fault. Also, when I try to free the memory I allocated for my test string I get a warning which says " 'free' called on a pointer to an unallocated object " Here is my code:

void reverseString(char* str, size_t size) {
    char *temp = (char*) malloc(sizeof(str) + 1);
    int j = size;
    for (int i = 0; i < size; i++) {
        temp[i] = str[j];
        j--;
    }
    for (int i = 0; i < size; i++) {
        str[i] = temp[i];
    }

    free(temp);
    return;
}


int main() {
    char* result = (char*)(malloc(sizeof(char) * 10));
    result = "Forty-two";

    reverseString(result, strlen(result));

    printf("%s", result);

    free(result);
    result = NULL;

    return 0;
}
Illya Kuzmych
  • 7
  • 2
  • 2
    `result = "Forty-two"` doesn't copy the string to the allocated memory, it re-assigns the pointer. – bereal Oct 31 '22 at 16:24
  • 1 - sizeof(str) returns size of pointer not length of the literal. 2 - Array index starts from 0. That's why j should starts with (size - 1) 3 - You are allocating memory from heap, use memset before do something. – embeddedstack Oct 31 '22 at 16:42
  • 1
    WRONG: sizeof(str). BETTER: `strlen(str)`. BEST: `int len = strlen(str); if (len < 1) { handle "bad string" }` ALSO WRONG: `result = "Forty-two";`. BETTER: `char * result = strdup("Forty Two");`. See [man strdup()](https://man7.org/linux/man-pages/man3/strdup.3.html) – paulsm4 Oct 31 '22 at 17:44
  • 1
    Also: Look here: https://stackoverflow.com/a/198264/3135317. You really don't need malloc/free at all. – FoggyDay Oct 31 '22 at 18:06

2 Answers2

1

On the second line, you should be using strlen instead of sizeof, because otherwise you will be allocating space for a character pointer and you need more than that.

CoffeeTableEspresso
  • 2,614
  • 1
  • 12
  • 30
Pedro Vieira
  • 11
  • 3
  • They should be using the parameter `size`. That's what it's there for. (But you're right that `sizeof` is not correct.) – rici Oct 31 '22 at 16:36
  • I changed my code to instead use strlen, and was able to properly populate the temp variable. I ran a debugger and saw that through it. However, I catch a segmentation fault when trying to popular the original str variable that I passed through on lines 12-14. What could be the cause of this? – Illya Kuzmych Oct 31 '22 at 16:51
  • @IllyaKuzmych did you change the line `result = "Forty-two";` yet? – CoffeeTableEspresso Oct 31 '22 at 17:38
  • Yes, I used memcpy to move the const char* "Forty-two" into the buffer "result" – Illya Kuzmych Oct 31 '22 at 17:53
0
  • sizeof(str) returns size of pointer not length of the literal.
  • Array index starts from 0. That's why j should starts with (size - 1)
  • You are allocating memory from heap, use memset before do something.

@bereal already says, if you want to understand more, please check this out :

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

int main() {
    char* result = (char*)(malloc(sizeof(char) * 10));
    memset(result, 0, 10);

    printf("Addr of result var : %p \n", result);

    result = "Re-assign";

    printf("Addr of result var : %p \n", result);
    return 0;
}

Maybe my solution gives an idea for you

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

void reverseString(char** str, size_t size) {
    char *temp = (char*) malloc(size + 1);
    memset(temp, 0, size + 1);
    int j = size - 1;
    for (int i = 0; i < size; i++) {
        temp[i] = str[0][j];
        j--;
    }

    //Change addr of holding str
    *str = temp;

    return;
}


int main() {
    char* result = "Forty-two";

    reverseString(&result, strlen(result));

    printf("%s", result);

    //result holds same addr with temp
    free(result);

    return 0;
}

But there are ways to solve this question more accurately.

embeddedstack
  • 362
  • 1
  • 13