using free() to free memory cause crash

Question

I'm trying to make a small library to handle strings as it's abnormally complicated to handle them in C.

I have a structure defined as such:

typedef struct _String
{
    unsigned int size;
    char *string;
} String;

It's quite simple, and allow me to dynamically change the array size (provided i use it correctly).

I have a function that's dedicated to creating this structure, as well as a function to free memory using a pointer to a String.

String *create_string(char *chr)
{
    String *str = calloc(1, sizeof(unsigned int) + sizeof(chr));
    str->string = chr;
    str->size = strlen(chr);

    return str;
}

void destroy_string(String *str)
{
    free(str);
}

But anyway, i'm having issues making a concatenation function which is defined as such:

bool concat_string_char(String *str, char *chr)
{
    // No use to continue since the passed String isn't initialized
    if (str->string == NULL) return false;

    // Storing the previous string pointer
    char *ptr = str->string;
    
    // Final size after concat
    int final_size = str->size + strlen(chr);

    // Allocating a new block of memory of size final_size * sizeof(char)
    str->string = calloc(1, final_size * sizeof(char));

    // Append each characters of orignal string
    for (int i = 0; i != str->size; i++)
    {
        str->string[i] = ptr[i];
    }

    // append each character of chr
    for (int i = 0; i != strlen(chr); i++)
    {
        str->string[str->size++] = chr[i];
    }

    // Free the memory allocated by the previous string -> Crash
    free(ptr);

    return true;
}

As i commented, a crash happen when i free the memory at the pointer used by the original string.

Includes:

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

you can try using the 3 functions above as follow (provided you comment free():

int main(void)
{
    String *str = create_string("Original");
    concat_string_char(str, " Concatenated");
    printf("%s\n", str->string);
    destroy_string(str);
    return 0;
}

replit: https://replit.com/@Mrcubix-Mrcubix/String-test#main.c

/EDIT: The Output string is indeed the one expected, the only issue here is to free this old pointer to not leak memory. END/

I tried using gdb to see if i could debug anything but as always, debugger have only ever been useful in cases where i couldn't find the location of crashes, never to figure out issues.

But anyway, it anyone would like to point out my mistake and explain in further details why it's wrong, i think it would improve my understanding of pointer in these kind of situations.

Answer 1

This is what you've got wrong:

String *create_string(char *chr)
{
    String *str = calloc(1, sizeof(unsigned int) + sizeof(chr));
    str->string = chr;
    str->size = strlen(chr);

    return str;
}

The first problem is here:

String *str = calloc(1, sizeof(unsigned int) + sizeof(chr));

You are allocating memory for the whole struct, including str->string. I get it, this prevents heap fragmentation but it also complicates manipulation.

Caling free on str->string will cause a segmentation fault because that address is invalid. You can only call free on str.

Second:

str->string = chr;

This is not copying the string, this is just assigning the pointer. That's plain wrong. You must copy using memcpy or similar:

memcpy(res->string, value, res->size);

Third: This may work:

String *create_string(char *chr)
{
    String *str = malloc(sizeof(String));
    str->size = strlen(chr);
    str->string = malloc(str->size);
    memcpy(res->string, value, res->size);
    return str;
}

And, if you want to add the terminating NULL character, try this:

void destroy_string(String *str)
{
    free(str->string);
    free(str);
}

Finally: You are not setting a terminating NULL character, have this in mind when printing (eg: using standard print functions).

If you want to add the terminating NULL character, change the constructor to this.

String *create_string(char *chr)
{
    String *str = malloc(sizeof(String));
    str->size = strlen(chr);
    str->string = malloc(str->size+1);
    memcpy(res->string, value, res->size);
    str->string[str->size] = '\0';
    return str;
}

Of course you need to take this into consideration in the concat function.

Note: You can avoid the second assignment by copying the null character from the source string, as all strings in C ar NULL-terminated (Thanks @0___________):

    memcpy(res->string, value, res->size+1);

Update: You are using calloc incorrectly:

str->string = calloc(1, final_size * sizeof(char));

The correct use is:

str->string = calloc(final_size, sizeof(char));