Unexpected results using mutex

Question

I have to determine on which positions it is founded a given value in an array of integers using N threads and display those values on the screen. I thought that i could solve this by using an array where i store the indexes, helped by a variable that will track the number of found positions. Been shared variables, i am using mutex. The problem is that i have duplicate positions on output. What I am doing wrong?

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

#define P 10
#define SIZE (sizeof(elementsList)/sizeof(elementsList[0]))



static int elementsList[] = {   1,2,3,4,5,6,7,8,3,10,11,12,3,14,15,16,17,
                1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,
                1,2,3,4,5,6,7,8,9,3,11,12,13,14,3,16,17,
                1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,
                1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,3   };

static int positions[SIZE];
pthread_mutex_t mutex;
volatile int count = 0;
static int value_to_be_found;
static pthread_t threads[P];

void *getPositions(void *arg){
    int *argPtr = arg;
    int start = *argPtr;
    int i;


    for(i = start; i < SIZE; i+= P){
        if(elementsList[i] == value_to_be_found){
            pthread_mutex_lock(&mutex);    
            count++;
            positions[count] = i;           
        pthread_mutex_unlock(&mutex);
       }
    }

}

int main(){

    int th;
    int ret;
    int i;
    printf("Enter the value to be found: \n");
    scanf("%d",&value_to_be_found);

    for(th = 0; th < P; th++){
        ret = pthread_create(&threads[th],NULL,&getPositions,&th);
        if(ret){
            printf("Error!\n");
            exit(-1);
        }
    }

    for(th = 0; th < P; th++){
        pthread_join(threads[th],NULL);
    }
    printf("The positions where % was found are: \n",value_to_be_found);
    for(i = 1; i <= count; i++){
        printf("%d. position #%d\n",i,positions[i]);
    }
    return 0;

}

EDIT: The input value is 3 The output isn't the same all the time, but here is one of the outputs:

position #2
position #12
position #84
position #36
position #36
position #43
position #53
position #8
position #48
position #19

What it should be displayed is:

position #70
position #2
position #12
position #43
position #53
position #84
position #36
position #8
position #48
position #19

Answer 1

There is a race condtion introduced in this line:

ret = pthread_create(&threads[th], NULL, &getPositions, &th);

as you are passing the address of th to each thread.

Then in the next iteration the value pointed to by the address changes. This may happend even before the thread created in the last iteration fetched the address, dereferenced it and stored the value locally by this line:

int start = *argPtr;

So it is not guaranteed that each thread has stored a different value for start.

---

The dirty solution to this would be to misuse the pointer passed to the thread function as an integer like:

ret = pthread_create(&threads[th], NULL, &getPositions, (void *)th);

and inside the thread function do

int start = (int) argPtr;

This works as long an int is shorter then a void *. To be on the secure see with this issue use intptr_t as type for th and start, as intptr_t is guaranteed to be the same size as void *.

---

The clean solution would be to define an array to hold the start values and pass the address of the i^th element to the thread function:

int starts[P] = {0};

...

int(main(void)
{
  ...

  for(th = 0; th < P; th++)
  {
    starts[th] = th;
    ret = pthread_create(&threads[th], NULL, &getPositions, &start[th]);

---

Also the getPositions() function misses a return statement, this however is not causing issues as pthread_join() doesn't use the value returned by any thread.

---

Also^2 : The code misses the protoype for exit(). Include <stdlib.h> to have it in place.