C - cast void* to structure in thrd_create()

Question

I know there's a lot of answered questions about casting void* to struct but I can't manage to get it work the right way.

Well I want to create a thread which will play music in background. I have a structure which gather the loaded music file array and the start and end index :

typedef unsigned char SoundID;
typedef unsigned char SongID;

typedef struct {
    Mix_Music *songs[9]; // array of songs
    SongID startId;      // index of first song to play
    SongID endId;        // index of last song to play
} SongThreadItem;

Then I want to play the songs by creating a thread and passing the function which actually plays the songs to the thread_create() function.

int play_songs(Mix_Music *songs[9], SongID startId, SongID endId, char loop){
    thrd_t thrd;
    SongThreadItem _item;
    SongThreadItem *item = &_item;

    memcpy(item->songs, songs, sizeof(item->songs));
    item->startId = startId;
    item->endId = endId;
    printf("item->startId is %i\n", item->startId);
    printf("item->endId is %i\n", item->endId);
    thrd_create_EC(thrd_create(&thrd, audio_thread_run, item));

    return 0;
}

int audio_thread_run(void *arg){
    SongThreadItem *item = arg; // also tried with = (SongThreadItem *)arg

    printf("item->startId is %i\n", item->startId);
    printf("item->endId is %i\n", item->endId);
    free(item);

    return 0;
}

Then I get the following output:

item->startId is 0
item->endId is 8
item->startId is 6
item->endId is 163

The value retrieved inside audio_thread_run() aren't the one expected. I don't know if I put enough code to let someone find my error, I try to make it minimal because it's part of a bigger project.

Thanks in advance for your help.

You're passing a pointer (`item`) to an automatic object (`_item`). By the time the thread runs that object is already dead. Calling `free` on something that wasn't allocated using `malloc` is also a no-no. — molbdnilo, Mar 03 '15 at 12:13

score 3 · Answer 1 · answered Mar 03 '15 at 12:11

SongThreadItem _item;
SongThreadItem *item = &_item; // bug

That's a problem there: you're giving the thread a pointer to a stack variable. The stack will get overwritten by pretty much anything going on in the main thread. You need to allocate dynamic memory here (with malloc), and take care of freeing it when no-longer needed (perhaps in the thread routine itself).

Other options would be a global structure that keeps track of all the active threads and their starting data, or something like that. But it will involve dynamic allocations unless the count of threads is fixed at compile time.

fork2execve · Accepted Answer · 2015-03-03T13:05:22.647

The thread runs asynchronously but you are passing it a pointer to SongThreadItem that is on the stack of the thread that calls play_songs().

If you have only a single thread calling play_songs() and this is not called again until you are done with the item, you can make the definition _item like this:

static SongThreadItem _item;

so that it is in the data segment and will not be overwritten.

If you don't know when and who will call play_songs() then just malloc the _item and free it in the thread when you are done:

...
SongThreadItem *item = (SongThreadItem *)malloc(sizeof(SongThreadItem));
...

The latter is usually the better idea. Think of it as passing the ownership of the data to the new thread. Of course production quality code should free the item if the thread creation fails.

Or, as usual, `item = malloc(sizeof *item);`. [No cast](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc), no repeated type name, much shorter. Much better. — unwind, Mar 03 '15 at 12:54

C - cast void* to structure in thrd_create()

2 Answers2