Homework: Freeing data in a struct

Question

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.

Answer 1

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;
}