C pass local variables to thread is broken

Question

#include <stdio.h>
#include <pthread.h>

typedef struct {
    int threadNum;
}thread_args;

void thread_func(void*vargp){
    thread_args*id=(thread_args*)vargp;
    printf("%i\n",id->threadNum);
}

int main() {
    for(int i=0;i<20;i++) {
        pthread_t id;
        thread_args args;
        args.threadNum=i;
        pthread_create(&id,NULL,thread_func,(void*)&args);
    }
    pthread_exit(NULL);
    return 0;
}

Adapted from https://www.geeksforgeeks.org/multithreading-c-2/.

So this is expected to output:

0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19

But shuffled in a random order to account for the concurrency of the threads.

The issue here is that it actually prints out this:

4
9
10
5
11
12
13
8
4
4
17
6
18
7
15
19
6
14
19
16

As you can see, there are duplicate numbers and 0-3 are just plain skipped.

I have done concurrency before in other frameworks before, and I have seen similar issues: what is happening here is that the i is being passed as a reference (I think!) and so when the for loop increments i, it is incremented in all thread argument variables.

How can I avoid this?

NOTE: Everything is linking 100% properly and I'm on macOS.

PS: Sorry if this is a duplicate, I'm not very experienced with this.

Answer 1

You are having an UB at your for loop. You are creating an variable called args, where you assign a value to it, pass as reference to your thread, for later execution, and destroy it at the end of your for loop. Then you do it again, possibling overwritting this region.

To solve that problem, I suggest this modification:

int main() {
    thread_args args[20] = {0};
    pthread_t id[20] = {0};

    for(int i=0;i<20;i++) {
        args[i].threadNum=i;
        pthread_create(&id[i],NULL,thread_func,(void*)&args[i]);
    }

    for(int i = 0; i < 20; i++)
        pthread_join(id[i], NULL);
    return 0;
}

Answer 2

This is, in fact, a race condition. You pass a void pointer to the argument struct, but (likely) the same memory address is reused for each argument struct. Therefore, when you later access it, you are likely to read modified memory. Try this:

#include <stdio.h>
#include <pthread.h>
#include <stdlib.h>
typedef struct {
    int threadNum;
}thread_args;

void thread_func(void* vargp){
    thread_args* id = (thread_args*)vargp;
    printf("%i\n", id->threadNum);

    free(vargp);
}

int main() {
    for(int i=0;i<20;i++) {
        pthread_t id;
        thread_args* args = malloc(sizeof(thread_args));
        args->threadNum = i;
        pthread_create(&id, NULL, thread_func, (void*)args);
    }
    pthread_exit(NULL);
    return 0;
}

Thanks to Kamil Cuk for pointing out another race condition.

Note that this snippet might still leak because the code never joins the threads, so the free() might never be called.