Why does free() cause a program crash in this code?

Question

I've already searched for an answer to this, but so far haven't come up with anything that answers the specific question, so asking here.

I have some C code that dynamically allocates space for a string and copies in the contents of a buffer that's sat in a local variable. This dynamically allocated string is then stored in a struct and used elsewhere in the program.

Now, when I'm done with the struct, I'm trying to be a good citizen and deallocate the string, however when I do so, without fail, the program crashes. As far as I can tell, I'm passing in the correct pointer to free(), and if I omit the call to free() then I've confirmed that I'm leaking memory, so what am I doing wrong?

Here's the gist of the code.

typedef struct Token {
    size_t length;
    const char* lexeme;

    // Other members
} Token;

// Original string is stored in a stack allocated static buffer:
static char buffer[1024];

// ... stuff here to fill buffer

// Then we generate the dynamic copy of the buffer contents
char* formattedMessage = realloc(NULL, sizeof(char) * strlen(buffer));

// Token is dynamically allocated as needed:
Token* token = realloc(NULL, sizeof(Token));
token->lexeme = strcpy(formattedMessage, buffer);
token->length = strlen(formattedMessage);

// ... Then we do other stuff (which all works, the message is stored in the struct and all is well)

// When we're done we call:
token->lexeme = free((void*)(token->lexeme));

// Which is where the program crashes.

Note that in the actual code, I do test that token->lexeme is a valid pointer to a string and checking the memory dump as I'm tracing through shows that the address being passed into free() is the correct address for the string.

Tearing my hair out here.

`char* formattedMessage = realloc(NULL, sizeof(char) * strlen(buffer));` is wrong. `strlen` returns the number of non-null characters in the string. The memory requirement is one more, for the null terminator. Then `strcpy(formattedMessage, buffer);` writes outside the allocated memory. Your program may have additional bugs. — Eric Postpischil, Jul 08 '23 at 00:28
Kind of an aside: Using `realloc()` to just allocate memory is weird and confusing. Using the return value from `strcpy()` is also very unusual. Keep your code as simple as you can. — pmacfarlane, Jul 08 '23 at 00:33
You cannot do that: `token->lexeme = strcpy(formattedMessage, buffer);` as `token->lexeme` ist not pointing to valid memory. Note that the previous line allocated memory for a `struct Token` but it **does not** allocate memory for the pointers inside the struct. — Pablo, Jul 08 '23 at 00:33
@pmacfarlane oh yes, you're right. I consider that bad style, very easy to miss. — Pablo, Jul 08 '23 at 00:40
You `realloc` on a existing pointer variable `Token` - that's not good practice, `realloc` into a temporary pointer variable, check if its legitimate pointer, then check the contents to make sure they are valid, if it is `NULL` do not assign it back to the original pointer variable `Token`. — t0mm13b, Jul 08 '23 at 06:53
@t0mm13b OP has `Token* token = realloc(NULL, sizeof(Token));`. `token` has no previous value as this is the _definition_ of `token`. For a re-allocation, your [comment](https://stackoverflow.com/questions/76640853/why-does-free-cause-a-program-crash-in-this-code/76641744#comment135124978_76640853) makes sense, yet this is a first time allocation for `token` - even though is calls `realloc()`. — chux - Reinstate Monica, Jul 08 '23 at 07:23
Thanks @chux-ReinstateMonica yes, using `realloc(NULL, ...)` is, not the right way to say this, "unusual" vs using `malloc(...)` unless there's a specific reason, despite the behaviour of `realloc(NULL,...)` is equivalent to `malloc(...)` - a case of overloading `realloc` - too many ways to "skin a cat" — t0mm13b, Jul 08 '23 at 07:33
@t0mm13b Since good questions oblige a user to take their problem code and filter down to a [mcve], things like weird `realloc(NULL, ...)` vs. common `malloc()` are excusable. IMHO, the only folks who make great questions already know the answer. (like teachers and lawyers) — chux - Reinstate Monica, Jul 08 '23 at 07:38
OP: Why are you doing this `token->lexeme = free((void*)(token->lexeme));` ? Member of the `Token`, `lexeme` is a `const char *` , why did you cast it to a `void *`? I would start there if I were you and fix this and debug it on that line. — t0mm13b, Jul 08 '23 at 07:47
@t0mm13b: The cast to void* is simply to avoid compiler warnings ... it'll work without but will complain that free expects void* as it's parameter. — Mark Edwards, Jul 08 '23 at 09:14
@pmacfarlane: The use of realloc is due to all allocations being performed by a separate function that handles initial allocation _and_ resizing of existing memory, hence the use of realloc throughout. It looks a little odd here, because I've simply condensed the code down to the bare minimum. (same as with my comment elsewhere about the line `token->lexeme = free((void*)(token->lexeme))` not working - that's an error I've introduced inadvertently in this boiled down code - in the actual code, the free() call is made via a function that returns NULL after the free takes place. — Mark Edwards, Jul 08 '23 at 09:18
@MarkEdwards " it'll work without but will complain that free expects void* as it's parameter. " --> That hints you are not using a C compiler. What compiler are you using? — chux - Reinstate Monica, Jul 08 '23 at 09:46
@chux-ReinstateMonica gcc bundled with MinGW w64 9.0, although to be clear, the complainant is my IDE (CLion) — Mark Edwards, Jul 08 '23 at 12:33

chux - Reinstate Monica · Accepted Answer · 2023-07-08T07:25:49.700

2

strcpy(a, b) needs a to point to memory of at least size strlen(b) + 1. realloc(NULL, sizeof(char) * strlen(buffer)) allocates one less than needed.

free() does not return anything. I would not expect token->lexeme = free((void*)(token->lexeme)); to even compile.

Easy enough to fix, yet with the new C23 making strdup() part of the standard library, consider using that to allcate and copy a string - or roll your own.

// Unnecessary casts removed

char* formattedMessage = strdup(buffer);
if (formattedMessage == NULL) {
  Handle_OutOfMemeory(); // TBD code
}

Token* token = realloc(NULL, sizeof token[0]);
if (token == NULL) {
  Handle_OutOfMemeory(); // TBD code
}
token->lexeme = formattedMessage;
token->length = strlen(formattedMessage);


// ... Then we do other stuff 

// When we're done we call:
free(token->lexeme)
free(token);

Style: malloc(size); is much more common than realloc(NULL, size);.

edited Jul 08 '23 at 07:25

answered Jul 08 '23 at 06:58

chux - Reinstate Monica

143,097
13
135
256

Sorry, the `token->lexeme = free((void*)(token->lexeme));` line is a result of my condensing the code down to bare minimum. In reality, the call to free() is actually performed in a separate function that returns NULL after the free is done. – Mark Edwards Jul 08 '23 at 09:17
Ah, this was it ... an off-by-one error. Allocating the extra character for the null terminator in the string was indeed the issue. Obvious now I look at it with a fresh pair of eyes ... yesterday, I'd been looking at it for so long I think I'd gone code blind. – Mark Edwards Jul 08 '23 at 09:24

Why does free() cause a program crash in this code?

1 Answers1