Trouble freeing memory in C with other functions

Question

I'm creating a program where a struct is created through this process:

TokenizerT *TKCreate(char *separators, char *ts) {
    TokenizerT * inu = malloc(sizeof(*inu));
    char * sts = malloc(strlen(ts)*sizeof(char));
    char * by = malloc(lim*sizeof(char));
    strcpy(by, yr);
    strcpy(sts, ts);
    inu->sep = by;
    inu->toks = sts;
    return inu;
}

I need to free the struct inu through another function, but my function below only seems to free up the memory associated with TokenizerT.sep

void TKDestroy(TokenizerT *tk) {
    free(tk->sep);
    free(tk->toks);
}

How do I free up tk.sep and tk.toks?

EDIT: free(tk) results in this error: "malloc: * error for object 0x7fff55662bd8: pointer being freed was not allocated * set a breakpoint in malloc_error_break to debug"

EDIT2: Struct definition

struct TokenizerT_ {
char * sep;
char * toks;
};

And

void TKDestroy(TokenizerT *tk) {
    free(tk->sep);
    free(tk->toks);
    free(tk);
}

results in the same error specified in EDIT 1

EDIT 3: I've also added my main method:

int main(int argc, char **argv) {

char * arr = argv[1];
char * y = argv[2];
TokenizerT jer = *TKCreate(arr, y);
TKDestroy(&jer);

return 0;
}

Answer 1

The reason that your code fails with that error is that &jer is the address of a local variable. It is the not address returned by the call to malloc.

TokenizerT jer = *TKCreate(arr, y);
TKDestroy(&jer);

So, jer is a local variable. The address returned by TKCreate is immediately de-referenced, and a copy of the struct is made. You simply fail to retain the address returned by TKCreate and it is leaked. Then when you attempt to call free on &jer, the address of a local variable, your environment correctly reports that you are passing to free an address that was not created by a call to malloc.

As I said in your previous question, it makes a lot more sense to return the struct by value than using dynamic allocation. On top of that, your string allocations are incorrectly performed. You must always allocate sufficient room for the null-terminator.

I would write your program like this:

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

typedef struct {
    char *sep;
    char *toks;
} TokenizerT;

TokenizerT TKCreate(const char *seps, const char *toks) 
{
    TokenizerT inu;

    inu.sep = malloc(strlen(seps)+1);
    strcpy(inu.sep, seps);

    inu.toks = malloc(strlen(toks)+1);
    strcpy(inu.toks, toks);

    return inu;
}

void TKDestroy(TokenizerT *tk) 
{
    free(tk->sep);
    free(tk->toks);
}

int main(int argc, char **argv) 
{
    TokenizerT jer = TKCreate(argv[1], argv[2]);
    TKDestroy(&jer);
    return 0;
}

Notes:

1. By definition, sizeof(char) is 1 so idiom dictates that it is omitted as a multiplicative factor in calls to malloc().
2. I've given the parameters to TKCreate the same names as the fields of the struct. This makes it easier to understand what is going on. What's more, your code looked plain wrong since it was ignoring one of the parameters.
3. As stated above, the struct is returned by value. This is conceptually easier to handle.

---

You might prefer to write a helper function to duplicate strings. If your compiler's runtime already has a function named strdup you might use that. Otherwise you can use this trivial implementation:

char *strdup(const char *str)
{
    char *result = malloc(strlen(str)+1);
    strcpy(result, str);
    return result;
}

If you only want to search for the null-terminator once you write it like this:

char *strdup(const char *str)
{
    size_t len = strlen(str)+1;
    char *result = malloc(len);
    memcpy(result, str, len);
    return result;
}

Then the code becomes:

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

char *strdup(const char *str)
{
    size_t len = strlen(str)+1;
    char *result = malloc(len);
    memcpy(result, str, len);
    return result;
}

typedef struct {
    char *sep;
    char *toks;
} TokenizerT;

TokenizerT TKCreate(const char *seps, const char *toks) 
{
    TokenizerT inu;
    inu.sep = strdup(seps);
    inu.toks = strdup(toks);
    return inu;
}

void TKDestroy(TokenizerT *tk) 
{
    free(tk->sep);
    free(tk->toks);
}

int main(int argc, char **argv) 
{
    TokenizerT jer = TKCreate(argv[1], argv[2]);
    TKDestroy(&jer);
    return 0;
}

If you are desperate to return a pointer to the struct, do it like this:

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

char *strdup(const char *str)
{
    size_t len = strlen(str)+1;
    char *result = malloc(len);
    memcpy(result, str, len);
    return result;
}

typedef struct {
    char *sep;
    char *toks;
} TokenizerT;

TokenizerT *TKCreate(const char *seps, const char *toks) 
{
    TokenizerT *inu = malloc(sizeof *inu);
    inu->sep = strdup(seps);
    inu->toks = strdup(toks);
    return inu;
}

void TKDestroy(TokenizerT *tk) 
{
    free(tk->sep);
    free(tk->toks);
    free(tk);
}

int main(int argc, char **argv) 
{
    TokenizerT *jer = TKCreate(argv[1], argv[2]);
    TKDestroy(jer);
    return 0;
}

Note particularly the difference in the way I call TKCreate and TKDestroy.

Finally, I have ignored all error checking on the calls to malloc(). In real production code you would not do that, but for the sake of clarity of exposition it is much better to omit it here.

Answer 2

Concerning your main, replace:

TokenizerT jer = *TKCreate(arr, y);
TKDestroy(&jer);

With:

TokenizerT *jer = TKCreate(arr, y);
TKDestroy(jer);

The reason being that your create-function returns a pointer, which you then pass to your destroy-function...

Answer 3

First of all, the malloc of "inu" seems not correct

TokenizerT * inu = malloc(sizeof(inu));

I believe it only get memory with 4 bytes (in 32-bit system)

It should be:

TokenizerT * inu = malloc(sizeof(TokenizerT ));

And as you mentioned -- "I need to free the struct inu"

I think the allocated "inu" is passed into "TKDestroy(TokenizerT *tk)"

then:

void TKDestroy(TokenizerT *tk) {
    free(tk->sep);
    free(tk->toks);
    free(tk) // this free is what you want to free "inu"

Answer 4

You just need to reverse the operations you did for allocation:

void TKDestroy(TokenizerT *tk) {
    free(tk->sep);  // for the 3rd malloc()
    free(tk->toks); // for the 2nd malloc()
    free(tk);       // for the 1st malloc()
}

BTW: you know that sizeof(char) is always 1 - per definition?

---

The error you get results from your wrong allocation of tk - you just allocated enough memory for a pointer, not for the whole struct. That will mess things up.