tags:

views:

293

answers:

6

I have a struct

struct request {
  int code;
  char *message;
};

that I'd like to free properly.

I have the following function to do that:

void free_request(struct request *req) {
  if (req->message != NULL) {
      free(req->message);
  }
  free(req);
  req = NULL;
}

The problem is that I get an "free(): invalid pointer"/segfault error from the compiler when I try to free a request that has been created using a string literal:

struct request *req;
req = malloc(sizeof(struct request));
req->message = "TEST";
free_request(req);

Since I want to create request structs in different places, once using literals (on the client side) and once using *chars that I read from a socket (on the server side) I was wondering if there is a function to make sure that I don't try to free the literals while still allowing me to free the message I have created using a malloc.

+2  A: 

It segfaults because the memory location containing "TEST" is (usually) read-only and not located on the heap (usually because it resides in some read-only section of the program). Given only a char* pointer, you won't be able to know if it points to a free()-able string or not. Instead, you should allocate a buffer for req->message and copy the characters.

char* str = "TEST";
int len = strlen(str);
req->message = malloc(len+1);
strcpy(req->message, str);

Or, you can use strdup() as suggested by zneak.

In silico
Actually, it doesn't matter if it is read only or not. All that matters is that it isn't a heap address, so it can't be freed.
torak
And it's not a heap address because it is usually put in a read-only section of the program.
In silico
@In silico: It could be put in a writable section of memory, and still not belong to the heap. The important questions are (a) is it the address of a section of memory that was allocated, and (b) has it been freed earlier.
David Thornley
Niklas, you didn't allocate those strings, so free-ing them isn't valid.
Jay
+17  A: 

There is no standard function that lets you know if a pointer was dynamically allocated or not. You should include a flag in your struct to inform yourself of it, or only use dynamically allocated strings (strdup is your friend in this case). Depending on your networking setup, it might be simpler to use strdup (well, to tell the truth, it is simpler to use the strdup at all).

With strdup:

struct message* req;
req = malloc(sizeof *req);
req->message = strdup("TEST");
free_request(req);

With a flag:

struct message
{
    int code;
    char* message;
    bool isStatical; // replace to 'char' if bool doesn't exist
};

void free_request(struct message* req)
{
    if (!req->isStatical) free(req->message);
    free(req);
}

struct message* req;
req = malloc(sizeof *req);
req->message = "TEST";
req->isStatical = 1;
free_request(req);

Also, on't forget to zero your allocated memory when you create an object. That could save you a lot of trouble.

req = malloc(sizeof *req);
memset(req, 0, sizeof *req);

That, and setting req to NULL from free_request won't have any effect. You either need to take a struct message** or do it yourself after the function calls.

zneak
Could use `calloc` instead of `malloc` and `memset`.
bstpierre
+1 for the strdup version
rmeador
Thanks for the exhaustive answer. I was looking for a solution that would work from the "inside" of the struct/free function, i.e. without the person using it to have to pay attention but I guess that's not easily possible. It's actually a small library so anything that would make using it more convenient would be helpful. I'll just make sure to mention in the documentation that you'd have to pass a pointer that is valid to be used by free.
Niklas
@Niklas: If you want to make it easier, you should make a function to create the request objects yourself, and strdup from there. That way your clients don't have to worry about managing your memory.
zneak
This approach is quite adhoc. Consider cases where you are using strings that aren't literal (known literal), and strings that are provided anonymously. See my post for a general solution.
Noah Watkins
+3  A: 

I would suggest adding a member to struct request to indicate whether request::message is dynamically allocated, and set that member at the same time you assign request::message, then check it before releasing the memory. It's a bit messy in C.

Note that it is not only string literals that would cause the problem, any pointer to data not dynamically allocated on the heap by malloc() or calloc() would fail, so simply detecting "if a char points to a string literal in C"* even if it could be done portably would not help.

Clifford
+2  A: 

There is no way to tell if you are using a string literal (Well, you can put string literals in a custom .section created by GCC, and then examine the string pointer to determine if it is contained in the .section of literals). However...there is a better way using a simple programming pattern.

Allocation With Literal

Normal case. A call to free(req) will work as expected: freeing the request structure.

struct *req;

req = malloc(sizeof(*req));
req->message = "TEST";

Allocation With Dynamic String

In the following, some_string is a string you wish to store as the request message. It can be either a literal, or a dynamically allocated. This allocates memory for the string when the struct itself is allocated (and will be freed automatically when the struct is freed).

struct *req;

req = malloc(sizeof(*req)+strlen(some_string)+1);
req->message = (char *)&req[1];
strcpy(req->message, some_string);

Freeing

free(req);

Edit: General Case

Note that the allocation scheme above for the dynamic string is general, it can be used even when you don't know if some_string is a literal or not. Thus, a single function that takes care of both cases, and freeing with free() rids you of special cases.

Noah Watkins
`req->message = "TEST";` will copy the pointer, and when you try to free it it will blow up (since "TEST" was not dynamically allocated), `req = malloc(sizeof(*req)); req->message = "TEST";` leaks (memory is allocated then the pointer is overwritten). You would have to copy "TEST" into the dynamic memory allocated
Bob Fincheimer
You should reconsider your statements. (1) I never suggested that you call `free(req->message)`. In my allocation scheme one should only ever free `struct request *`, not its `message` member. (2), `req->message = "TEST"` certainly does not leak memory. If you read my post more carefully then you would notice that I was referring to *statically allocated strings*, which *cannot* be freed, thus cannot be leaked. Finally, if you look at my 2nd section, you'll realize where dynamic memory comes into play. Please, let me know if you need more explanation on how it works.
Noah Watkins
I see, thanks. A little complicated to follow, but I see how it works now. Thanks
Bob Fincheimer
Simple isn't always better. It's also a common programming pattern in the Linux kernel.
Noah Watkins
A: 

If you're just trying to make sure that malloc'ed memory is freed, you can call

realloc(req->message,(size_t)0)

If the memory library implementation is robust, it should work.

oops
The OP did exactly this, but via the call to `free()`. The problem is that the memory wasn't allocated by `malloc()`, so calling `free()` or `realloc()` on it invoked the demons of Undefined Behavior.
RBerteig
I believe I said "if the memory library implementation is robust"...Ever implemented malloc, realloc, free, etc? You have to check every block passed to make sure that you own it. Unlike free (which has a void return), realloc has a (void *) return. Any implementation of realloc is supposed to return NULL if it can't reallocate the buffer. Yes, that behavior is defined and you can test it.BTW, Niklas didn't ask for comments about his design. He asked a simple questtion. The answer is that you can do it with a proper library.
oops
@oops This is completely wrong. You said "that behavior is defined". Not, and I quote, "if ptr does not match a pointer earlier returned by the calloc, malloc, or realloc function". C99 7.20.3.4, paragraph 3. And I did implement a `malloc` library, by the way. It didn't check you passed blocks that were heap-allocated because behavior was undefined if you did.
Pascal Cuoq
@PC You're right, I forgot about that. My implementation was required to match the spec for returning null if the buffer couldn't be realloc'ed. Since it tracked malloc'ed regions, out of range pointers were obvious. On free, it had to throw an exception but realloc could fail gracefully.
oops
A: 

Have a look at:

struct request *req;
req = calloc(1,sizeof(struct request));
strcpy(req->message = malloc(strlen("TEST")+1),"TEST");
free_request(req);

Its strictly ANSI C conform. strdup isnt ANSI C.

req = NULL; 

is redundant.