CODEWITHSUNDEEP
cstruct
bumbleBumble
bumbleBumble

Reputation: 83

Homework: Freeing data in a struct

I'm just learning to use valgrind and c, and I have an invalid free() output when trying to free data from a struct. I believe it's because data is not being freed from the struct correctly.

This is my struct:

typedef struct song_
{
    char *artist;
    char *title;
    mtime *lastPlayed;
} song;

And this is the function which tries to free it:

void songDelete(song *s)
{
    //artist
    free(s->artist) ;
    //title
    free(s->title) ;
    //time
    if(NULL != s->lastPlayed)
        mtimeDelete(s->lastPlayed) ;
    //song
    free(s);
}

mtime and mtimeDelete are some user-defined variables and methods, but I feel like they're not relevant to my question. I know it's wrong to ask someone to do my homework for me, I'd just like a push in the right direction if possible.

Upvotes: 5

Views: 139

Answers (3)

paxdiablo
paxdiablo

Reputation: 881653

No, that's definitely the right way to do it.

So, if valgrind is complaining, it's probably because the values in artist, title or lastPlayed are not actually valid pointers.

That's the first thing I'd be checking.

In other words, make sure what you put in there are valid pointers. Simply creating a song with:

song *AchyBreakyHeart = malloc (sizeof (song));

won't populate the fields (they'll be set to arbitrary values). Similarly,

AchyBreakyHeart->artist = "Bill Ray Cyrus";

will populate it with a string constant rather than a valid pointer in the heap.

The ideal thing would be to have a "constructor", similar to the destructor you've provided, something like:

song *songCreate (char *artist, char *title, mtime *lastPlayed) {
    song *s = malloc (sizeof (song));
    if (s == NULL) return NULL;

    s->artist = strdup (artist);
    if (s->artist == NULL) {
        free (s);
        return NULL;
    }

    s->title = strdup (title);
    if (s->title == NULL) {
        free (s->artist);
        free (s);
        return NULL;
    }

    s->lastPlayed = mtimeDup (lastPlayed);
    if (s->lastPlayed == NULL) {
        free (s->title);
        free (s->artist);
        free (s);
        return NULL;
    }

    return s;
}

This guarantees that the object is either fully constructed or not constructed at all (ie, no half-states).

Better yet would be to adapt the constructor/destructor pair to handle NULLs in conjuction with each other, to simplify the pair. First, a slightly modified destructor, the only change being that it can accept NULL and ignore it:

void songDelete (song *s) {
    // Allow for 'songDelete (NULL)'.

    if (s != NULL) {
        free (s->artist);  // 'free (NULL)' is valid, does nothing.
        free (s->title);
        if (s->lastPlayed != NULL) {
            mtimeDelete (s->lastPlayed) ;
        }
        free (s);
    }
}

Next, the constructor which, rather than trying to remember what has been allocated, instead sets them all to NULL initially and just calls the destructor if something goes wrong:

song *songCreate (char *artist, char *title, mtime *lastPlayed) {
    // Create song, null all fields to ease destruction,
    //   then only return it if ALL allocations work.

    song *s = malloc (sizeof (song));
    if (s != NULL) {
        s->artist = s->title = s->lastPlayed = NULL;

        s->artist = strdup (artist);
        if (s->artist != NULL) {
            s->title = strdup (title);
            if (s->title != NULL) {
                s->lastPlayed = mtimeDup (lastPlayed);
                if (s->lastPlayed != NULL) {
                    return s;
                }
            }
        }
    }

    // If ANY allocation failed, destruct the song and return NULL.

    songDelete (s);
    return NULL;
}

Upvotes: 3

Paul Draper
Paul Draper

Reputation: 83283

Your code appears correct.

Make sure that your struct is initialized correctly (with pointers set to NULL or a valid value).

int main() {
    song_ *s = calloc(sizeof(song_));
    free(s);
    return 0;
}

Upvotes: 1

brokenfoot
brokenfoot

Reputation: 11629

Did you create an object/pointer to your Struct using malloc (heap allocation)? or just song s; (stack allocation) ??

You can't free it if it hasn't been malloc'd, or in other words you can't free a variable on stack, it has to be on the heap.

Upvotes: 0

Related Questions