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.

CodePudding user response：

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));

CodePudding user response：

Your code is invalid in many places:

1. Use size_t for sizes

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

It might not allocate enough space for the struct as it does not know anything about the padding

    str->string = chr;

You need to copy it. But you do not have any memory allocated for your string. The assignment does not allocate memory for it or copy the string content.

In the concat_string_char you try to free the pointer which was not dynamically allocated - thus crash.

I would implement it other way:

typedef struct String
{
    size_t size;
    char string[];
} String;


String *create_string(const char * restrict chr)
{
    size_t len = strlen(chr);
    String *str = malloc(sizeof(*str)   len   1);
    if(str)
    {
        str->size = len;
        memcpy(str -> string, chr, len   1);
    }

    return str;
}

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

String *concat_string_char(String *str, char *chr)
{
    size_t len;
    if(str)
    {
        str = realloc(sizeof(*str)   str > size   (len = strlen(chr))   1);
        if(str)
        {
            strcpy(str -> data   str -> size, chr);
            str -> size  = len;
        }
    }
    return str;
}

CodePudding user response：

I don't think that the ptr-variable is leaking memory at all. As it's just a pointer to the beginning of the actual string and it's placed on the stack and not the heap, as a single pointer is just an integer and those free themselves too if you declare them in functions. If there's memory leak it's probably coming from here:
str->string = calloc(1, final_size * sizeof(char));
as your allocating new memory on the heap for your str struct without freeing what was stored there before. So you could try free(str); before allocating memory for the concatinated string. You're allocating memory and getting a pointer to it from calloc() but the memory of the not concatinated string is still on the heap, you just don't have pointer to it anymore.