CODEWITHSUNDEEP
cmemoryfree
Keith Miller
Keith Miller

Reputation: 1345

C using free correctly

I believe I am doing this right but wanted to make sure. In a program I am using two pointers that point to allocated memory returned by a function. When I finish using that memory I free() it, but then I use those same pointers to point at a new allocated memory space. Here is my program to give an example of what I mean (Commented to show my thought process):

int main(void)
{
    char *data, *url;
        int i = 1;

    while(i)
    {
        printf("Enter URL: ");
        if(!(url = getstr())) return 1;                                  //url now points to allocated memory from getstr();
        if(strlen(url) <= 0) i = 0;
        if(data = readsocket(url, 80, "http")) printf("\n%s\n\n", data); //data now points to allocated memory from readsocket();
        else printf("\n\n");
        free(url);                                                       //free allocated memory that url points to url
        free(data);                                                      //free allocated memory that data points to data
    }
    return 0;
}

Is this correct, or is there a better more generally preffered method of doing this? Or am I just completely doing it wrong?

Upvotes: 1

Views: 284

Answers (4)

jleahy
jleahy

Reputation: 16895

Assuming your functions getstr and readsocket malloc the memory internally then this is perfectly fine.

My only suggestion is that it's often helpful to allocate memory in the same scope that you free it, this often helps reason about when things need to be freed.

For example:

url = malloc(URL_MAX_LEN);
if (!url) return 1;
if (!getstr(url)) {
    free(url);
    return 1;
}
/* do something with url */
free(url);

It can be nice to do this using goto, if you have lots of objects and exit points.

object1 = malloc(OBJECT1_SIZE);
/* do work, if there's an error goto exit1 */
object2 = malloc(OBJECT2_SIZE);
/* do work, if there's an error goto exit2 */
object3 = malloc(OBJECT3_SIZE);
/* do work, if there's an error goto exit3 */
exit3:
free(object3);
exit2:
free(object2);
exit1:
free(object1);
return 1;

If your objects get more complex and require a lot of work to create and destroy then you can wrap malloc and free, but it's critical you apply exactly the same rules to your create/destroy functions as you would to malloc/free to avoid leaks. As a matter of good practice you should always have a destroy to match a create, even if it just wraps free. It's much harder to go wrong that way.

struct x *create_x() {
    struct x *p = malloc(10);
    p->val1 = 7;
    return p;
}
void destroy_x(struct x *p) {
    free(p);
}

Upvotes: 5

Jan Spurny
Jan Spurny

Reputation: 5537

Here's some things I've noticed:

  1. You're using i to control the loop, but when you set i = 0, the program has to go all the way to closing curly bracket. I'd recommend using while (1) and break statements and getting rid of i variable:

    while(1) {
    ...
    if (strlen(url) <= 0)
        break;
    ...
}

  2. you are using if (data = readsocket(url, 80, "http")) which is not technically wrong, but it is a bit misleading as someone may mistake it for comparison if (data == readsocket(url, 80, "http"). This is almost a "pattern" in C programming and it's generally a good idea to write just like you did on if (!url = getstr())) line:

    if ((data = readsocket(url, 80, "http")) != NULL) ...

    or

    if (!(data = readsocket(url, 80, "http"))) ...

  3. this is more a "style"-related, but it is better to write separate statements on separate lines, so don't write

    if (!(url = getstr())) return 1

    but rather

    if (!(url = getstr()))
    return 1;

  4. as for free-ing memory, you're doing just fine

Upvotes: 1

Some programmer dude
Some programmer dude

Reputation: 409482

If the functions getstr and readsocket allocates memory using e.g. malloc then it's what you are doing is okay.

Upvotes: 2

perreal
perreal

Reputation: 98118

There is no problem in your code, but you can pre-allocate memory for data and url and pass these as arguments to getstr and readsocket functions. This will be more efficient.

Upvotes: 2

Related Questions