Help with destructor function using free()


#1

What I would like to do:

#include <stdio.h>;

void destroy(char** x) 
{
    free(*x);  //free up the passed pointer
    *x = NULL;
    return;
}

int main()
{

    char *ptr;
    char A = 6;

    ptr = malloc(sizeof(char));
    ptr = &A;
    printf("ptr = %ld\r\n", &ptr);
    destroy(&ptr);
    printf("ptr = %ld\r\n", ptr);  //expect to be NULL here

    return 0;

}

Ok, this is a contrived example. My destroy() function is going to be a bit more complex. The problem is the compiler doesn’t like what I’m doing. I’m quite sure this kind of thing is done all the time. I feel like I’m missing some subtlety here.

Help please?


#4

First of all adding the tags added the semicolons to the include lines
Second, yeah copy paste error there
Third, the %ld was so that I could print the ADDRESS of the ptr variable not it’s value. I just took a guess at what it would be since I’m running this code on an online compiler/simulator
Forth, this is a contrived example to illustrate the point (as is proper etiquette). My code will be freeing a allocated structure.
Fifth, this is your opinion and if you search the interwebs you will find it to be a hotly debated topic. I think it is good practice so that you can prevent the freed pointer from being accidentally reused. “defensive programming techniques”
Sixth, you can print the value of NULL at will and without error
Seventh, your code doesn’t work. I ran it and indeed the passed pointer does NOT get freed. The value in main is the same before and after. The reason being (I suspect) is the variable being freed in the destroy function is a local copy in that function. i.e. “char *x” itself is stored at a location on the stack and free() is operating on that location NOT the location of ptr in main().

[Redacted by Zed.]


#6

The code in Exercise 17 in Learn C The Hard Way might be insightful. It shows you how to allocate structures and destroy them. Here’s the official unaltered lecture code:

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>

#define MAX_DATA 512
#define MAX_ROWS 100

struct Address {
    int id;
    int set;
    char name[MAX_DATA];
    char email[MAX_DATA];
};

struct Database {
    struct Address rows[MAX_ROWS];
};

struct Connection {
    FILE *file;
    struct Database *db;
};

void die(const char *message)
{
    if (errno) {
        perror(message);
    } else {
        printf("ERROR: %s\n", message);
    }

    exit(1);
}

void Address_print(struct Address *addr)
{
    printf("%d %s %s\n", addr->id, addr->name, addr->email);
}

void Database_load(struct Connection *conn)
{
    int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
    if (rc != 1)
        die("Failed to load database.");
}

struct Connection *Database_open(const char *filename, char mode)
{
    struct Connection *conn = malloc(sizeof(struct Connection));
    if (!conn)
        die("Memory error");

    conn->db = malloc(sizeof(struct Database));
    if (!conn->db)
        die("Memory error");

    if (mode == 'c') {
        conn->file = fopen(filename, "w");
    } else {
        conn->file = fopen(filename, "r+");

        if (conn->file) {
            Database_load(conn);
        }
    }

    if (!conn->file)
        die("Failed to open the file");

    return conn;
}

void Database_close(struct Connection *conn)
{
    if (conn) {
        if (conn->file)
            fclose(conn->file);
        if (conn->db)
            free(conn->db);
        free(conn);
    }
}

void Database_write(struct Connection *conn)
{
    rewind(conn->file);

    int rc = fwrite(conn->db, sizeof(struct Database), 1, conn->file);
    if (rc != 1)
        die("Failed to write database.");

    rc = fflush(conn->file);
    if (rc == -1)
        die("Cannot flush database.");
}

void Database_create(struct Connection *conn)
{
    int i = 0;

    for (i = 0; i < MAX_ROWS; i++) {
        // make a prototype to initialize it
        struct Address addr = {.id = i,.set = 0 };
        // then just assign it
        conn->db->rows[i] = addr;
    }
}

void Database_set(struct Connection *conn, int id, const char *name,
        const char *email)
{
    struct Address *addr = &conn->db->rows[id];
    if (addr->set)
        die("Already set, delete it first");

    addr->set = 1;
    // WARNING: bug, read the "How To Break It" and fix this
    char *res = strncpy(addr->name, name, MAX_DATA);
    // demonstrate the strncpy bug
    if (!res)
        die("Name copy failed");

    res = strncpy(addr->email, email, MAX_DATA);
    if (!res)
        die("Email copy failed");
}

void Database_get(struct Connection *conn, int id)
{
    struct Address *addr = &conn->db->rows[id];

    if (addr->set) {
        Address_print(addr);
    } else {
        die("ID is not set");
    }
}

void Database_delete(struct Connection *conn, int id)
{
    struct Address addr = {.id = id,.set = 0 };
    conn->db->rows[id] = addr;
}

void Database_list(struct Connection *conn)
{
    int i = 0;
    struct Database *db = conn->db;

    for (i = 0; i < MAX_ROWS; i++) {
        struct Address *cur = &db->rows[i];

        if (cur->set) {
            Address_print(cur);
        }
    }
}

int main(int argc, char *argv[])
{
    if (argc < 3)
        die("USAGE: ex17 <dbfile> <action> [action params]");

    char *filename = argv[1];
    char action = argv[2][0];
    struct Connection *conn = Database_open(filename, action);
    int id = 0;

    if (argc > 3) id = atoi(argv[3]);
    if (id >= MAX_ROWS) die("There's not that many records.");

    switch (action) {
        case 'c':
            Database_create(conn);
            Database_write(conn);
            break;

        case 'g':
            if (argc != 4)
                die("Need an id to get");

            Database_get(conn, id);
            break;

        case 's':
            if (argc != 6)
                die("Need id, name, email to set");

            Database_set(conn, id, argv[4], argv[5]);
            Database_write(conn);
            break;

        case 'd':
            if (argc != 4)
                die("Need id to delete");

            Database_delete(conn, id);
            Database_write(conn);
            break;

        case 'l':
            Database_list(conn);
            break;
        default:
            die("Invalid action: c=create, g=get, s=set, d=del, l=list");
    }

    Database_close(conn);

    return 0;
}

With that said, I can let someone else help you from here out.


#7

Found the answer on StackOverflow:

https://stackoverflow.com/questions/13938204/what-does-free-do-to-a-pointer-passed-by-value-to-a-function

Basically the idea is to pass the pointer to be freed as a double-pointer so you can access the value it’s pointing to after free. I tried this out and it works well.


#8

You can pass a regular pointer too. You can see in the code I posted that it works. Check out the Database_close function.

But I should be clear, it’s hard to be more specific without seeing the working structures you are working with. The code you posted doesn’t reflect what you are actually trying to do.


#9

In the example you posted in the Database_close function, try to set conn to NULL. Then test its value back in main(). You will see that it is not NULL but in fact, still it’s original value. Passing the pointer in as reference allows you to set it to NULL after free(). Why is this (in my opinion) a good practice? Because elsewhere in your code, as normal, you should test if a pointer is NULL before using it. If you didn’t do this the pointer is still set to a value after free() but is not valid memory. This can be dangerous. Now I didn’t post this to start an argument about setting freed pointers to NULL. Everyone has their opinion and there probably isn’t a “right” answer. My problem was that the pointer was not getting set to NULL and I didn’t understand why or at least how to do it correctly.

Today is a good day. I hope we all learned something new.


#10

I see where you are going with that. Sorry for my lapse in comprehension. Sounds like you found the right answer in the double pointers. That would let you change the contents of the data pointed to instead of just changing the variable that was passed to the function by value. Though it appears the free function still operates properly, in valgrind the Ex17 code shows all memory freed.

(shrugs) Thanks for the educational moment.


#11

Hey Everyone, I locked this thread as it got a little heated. Please move on.