C code stack corruption changing variable

Question

I'm hoping that someone can help me out. I have not written much in C code in over a decade and just picked this back up 2 days ago so bear with me please as I am rusty. THANK YOU!

What:

I'm working on creating a very simple thread pool for an application. This code is written in C on CodeBlocks using GNU GCC for the compiler. It is built as a command line application. No additional files are linked or included.

The code should create X threads (in this case I have it set to 10) each of which sits and waits while watching an array entry (identified by the threads thread index or count) for any incoming data it might need to process. Once a given child has processed the data coming in via the array there is no need to pass the data back to the main thread; rather the child should simply reset that array entry to 0 to indicate that it is ready to process another input. The main thread will receive requests and will dole them out to whatever thread is available. If none are available then it will refuse to handle that input.

For simplicity sake the code below is a complete and working but trimmed and gutted version that DOES exhibit the stack overflow I am trying to track down. This compiles fine and initially runs fine but after a few passes the threadIndex value in the child thread process (workerThread) becomes corrupt and jumps to weird values - generally becoming the number of milliseconds I have put in for the 'Sleep' function.

What I have checked:

• The threadIndex variable is not a global or shared variable.
• All arrays are plenty big enough to handle the max number of threads I am creating.
• All loops have the loopvariable reset to 0 before running.
• I have not named multiple variables with the same name.
• I use atomic_load to make sure I don't write to the same global array variable with two different threads at once please note I am rusty... I may be misunderstanding how this part works
• I have placed test cases all over to see where the variable goes nuts and I am stumped.

Best Guess

All of my research confirms what I recall from years back; I likely am going out of bounds somewhere and causing stack corruption. I have looked at numerous other problems like this on google as well as on stack overflow and while all point me to the same conclusion I have been unable to figure out what specifically is wrong in my code.

#include<stdio.h>
//#include<string.h>
#include<pthread.h>
#include<stdlib.h>
#include<conio.h>
//#include<unistd.h>
#define ESCAPE 27

int maxThreads = 10;
pthread_t tid[21];
int ret[21];
int threadIncoming[21];
int threadRunning[21];

struct arg_struct {
    char* arg1;
    int arg2;
};
//sick of the stupid upper/lowercase nonsense... boom... fixed
void* sleep(int time){Sleep(time);}


void* workerThread(void *arguments)
{
    //get the stuff passed in to us
    struct arg_struct *args = (struct arg_struct *)arguments;
    char *address = args -> arg1;
    int threadIndex = args -> arg2;
    //hold how many we have processed - we are unlikely to ever hit the max so no need to round robin this number at this point
    unsigned long processedCount = 0;
    //this never triggers so it IS coming in correctly
    if(threadIndex > 20){
        printf("INIT ERROR! ThreadIndex = %d", threadIndex);
        sleep(1000);
    }
    unsigned long x = 0;
    pthread_t id = pthread_self();
    //as long as we should be running
    while(__atomic_load_n (&threadRunning[threadIndex], __ATOMIC_ACQUIRE)){
        //if and only if we have something to do...
        if(__atomic_load_n (&threadIncoming[threadIndex], __ATOMIC_ACQUIRE)){


            //simulate us doing something
            //for(x=0; x<(0xFFFFFFF);x++);
            sleep(2001); 
            //the value going into sleep is CLEARLY somehow ending up in index because you can change that to any number you want 
            //and next thing you know the next line says "First thread processing done on (the value given to sleep)
            printf("\n First thread processing done on %d\n", threadIndex);




            //all done doing something so clear the incoming so we can reuse it for our next one
            //this error should not EVER be able to get thrown but it is.... something is corrupting our stack and going into memory that it shouldn't
            if(threadIndex > 20){ printf("ERROR! ThreadIndex = %d", threadIndex); }
            else{ __atomic_store_n (&threadIncoming[threadIndex], 0, __ATOMIC_RELEASE); }
            //increment the processed count
            ++processedCount;
        }
        else{Sleep(10);}
    }
    //no need to do atomocity I don't think for this as it is only set on the exit and not read till after everything is done
    ret[threadIndex] = processedCount;
    pthread_exit(&ret[threadIndex]);
    return NULL;
}



int main(void)
{
    int i = 0;
    int err;
    int *ptr[21];
    int doLoop = 1;
    //initialize these all to set the threads to running and the status on incoming to NOT be processing
    for(i=0;i < maxThreads;i++){
            threadIncoming[i] = 0;
            threadRunning[i] = 1;
    }
    //create our threads
    for(i=0;i < maxThreads;i++)
    {
        struct arg_struct args;
        args.arg1 = "here";
        args.arg2 = i;
        err = pthread_create(&(tid[i]), NULL, &workerThread, (void *)&args);
        if (err != 0){ printf("\ncan't create thread :[%s]", strerror(err)); }
    }
    //loop until we hit escape
    while(doLoop){
        //see if we were pressed escape
        if(kbhit()){ if(getch() == ESCAPE){ doLoop = 0; } }
        //just for testing - actual version would load only as needed
        for(i=0;i < maxThreads;i++){
            //make sure we synchronize so we don't end up pointing into a garbage address or half loading when a thread accesses us or whatever was going on
            if(!__atomic_load_n (&threadIncoming[i], __ATOMIC_ACQUIRE)){
                __atomic_store_n (&threadIncoming[i], 1, __ATOMIC_RELEASE);
            }
        }
    }
    //exiting...
    printf("\n'Esc' pressed. Now exiting...\n");
    //call to end them all...
    for(i=0;i < maxThreads;i++){ __atomic_store_n (&threadRunning[i], 0, __ATOMIC_RELEASE); }
    //join them all back up - if we had an actual worthwhile value here we could use it
    for(i=0;i < maxThreads;i++){
        pthread_join(tid[i], (void**)&(ptr[i]));
        printf("\n return value from thread %d is [%d]\n", i, *ptr[i]);
    }
    return 0;
}

Output

Here is the output I get. Note that how long it takes before it starts going crazy does seem to possibly vary but not much.

Output Screen with Error

Answer 1

I don't trust your handling of args, there seems to be a race condition. What if you create N threads before the first one of them gets to run? Then the first thread created will probably see the args for the N:th thread, rather than for the first, and so on.

I don't believe there's a guarantee that automatic variables used in a loop like that are created in non-overlapping areas; after all they go out of scope with each iteration of the loop.