Basic posix thread argument passing issue

Question

I am trying to get into the world of threads and am having some trouble. The code below works every once in a while but seems to be completely random. Giving it the same input keeps giving me different results and I am quite confused. Sometimes PrintHello() prints out the arguments and other times it prints garbage and sometimes it just segfaults.

#define NUM_THREADS 5
char *prompt = "% ";

struct thread_data{
    int thread_id;
    //int sum;
    char *message;
};

void *PrintHello(void *threadarg)
{   
   struct thread_data *local_data;
   local_data = (struct thread_data *) threadarg;
   int taskid = local_data->thread_id;
   const char *arguments = local_data->message;
   fprintf(stderr, "Hello World! It's me, thread %s!\n", arguments);
   pthread_exit(NULL);
}

PrintHello() is where I think the issue is.

int main()
{
    int pid;
    //int child_pid;
    char line[81];
    char *token;
    char *separator = " \t\n";
    char **args;
    char **args2;
    char *hp;
    char *cp;
    char *ofile;
    int i;
    int j, h, t, rc;

    args = malloc(80 * sizeof(char *));
    args2 = malloc(80 * sizeof(char *));

    signal(SIGINT, SIG_IGN);

    while (1) {
        fprintf(stderr, "%s", prompt);
        fflush(stderr);

        if (fgets(line, 80, stdin) == NULL)
            break;

       /* get rid of the '\n' from fgets */
        if (line[strlen(line) - 1] == '\n'){
               line[strlen(line) - 1] = '\0';
    }
    // split up the line

    i = 0;
    while (1) {
        token = strtok((i == 0) ? line : NULL, separator);
        if (token == NULL)
            break;
        args[i++] = token;
    }

    args[i] = NULL;
    ofile = args[i-1];
    printf("%s\n", ofile);

The stuff above this is just tokenizing the input and works fine.

    struct thread_data thread_data_array[i];
    pthread_t threads[i];



    for(t=0; t<i; t++){

        thread_data_array[t].thread_id = t;
        thread_data_array[t].message = args[t];

        rc = pthread_create(&threads[t], NULL, PrintHello, (void *)&thread_data_array[t]);
        if (rc){
            printf("ERROR; return code from pthread_create() is %d\n", rc);
            exit(-1);
        }
  }
}      
}

Answer 1

There are several issues with your code, but I'll focus on the key ones that affect stability.

1. You're not checking the return values of malloc(). If the return value is NULL, it means the operation failed, and you have to either re-try, or start cleaning up all dynamically allocated memory from malloc(), calloc(), strdup(), etc, and finish up with your program gracefully. Attempting to dereference NULL (ie: use the pointer from the failed memory allocation call) will crash your program.
2. Your program doesn't account for zero valid arguments provided (ie: just hitting ENTER at the prompt. Change ALL newline instances to '\0', and continue.
3. Also, count the number of tokens you've discovered. Good practise, and helps you check if no valid input was found.
4. Consider reading up on starting threads in a detached state versus a joinable state. The biggest problem in your code is that you start all the threads, and then your while loop immediately executes again, and re-assigns new values to the thread[] array. Also, the same argument is passed to them while still in use (thread_data_array[t]), and you have no mutexes to protect them. Also, if your program's main() exits early, then all running threads get killed off immediately, and don't get to finish.
5. You should pthread_join() on joinable threads to ensure you wait until they complete before you proceed.
6. You provide no way to exit the program without either using CTRL+C or crashing it. Not a good idea.
7. Note that the threads don't necessarily execute in the order you created them, though they luckily did in this case. You'll need to learn about barriers, condition variables (condvars) and mutexes to do more advanced synchronization handling.
8. You're missing a lot of important header files. Surprised your code compiled.
9. Learn how to debug your code with gdb. In this case, I compiled it via gcc test.c -lpthread -O0 -ggdb, then stepped through the code via the "next" n command after starting it in gdb with run. It makes your life a lot easier.

---

Updated Code Listing

---

#include <stdio.h>
#include <signal.h>
#include <pthread.h>
#include <stdlib.h>
#include <string.h>

#define NUM_THREADS 5
#define BUF_LEN (80)
char *prompt = "% ";

struct thread_data{
    int thread_id;
    //int sum;
    char *message;
};

void *PrintHello(void *threadarg)
{   
    struct thread_data *local_data;
    local_data = (struct thread_data *) threadarg;
    int taskid = local_data->thread_id;
    const char *arguments = local_data->message;
    fprintf(stderr, "Hello World! It's me, thread %s!\n", arguments);
    pthread_exit(NULL);
}

int main()
{
    int pid;
    //int child_pid;
    char line[81];
    char *token;
    char *separator = " \t\n";
    char **args;
    char **args2;
    char *hp;
    char *cp;
    char *ofile;
    int i;
    int j, h, t, rc;

    args = malloc(BUF_LEN * sizeof(char *));    // ISSUE: Can fail. Check return value.
    args2 = malloc(BUF_LEN * sizeof(char *));   // ISSUE: Can fail.

    signal(SIGINT, SIG_IGN);

    while (1)
    {
        fprintf(stderr, "%s", prompt);
        fflush(stderr);

        if (fgets(line, BUF_LEN, stdin) == NULL)
        {
            break;
        }

        /* get rid of the '\n' from fgets */
        /*
        if (line[strlen(line) - 1] == '\n'){
            line[strlen(line) - 1] = '\0';
        }
        */
        for ( t = 0; t < BUF_LEN; t++ )
        {
            if ( line[t] == '\n' )
            {
                line[t] = '\0';
            }
        }

        // split up the line
        i = 0;
        int numTokens = 0;
        while (1) {
            token = strtok((i == 0) ? line : NULL, separator);
            if (token == NULL)
            {
                break;
            }
            args[i++] = token;
            numTokens++;
        }
        // Abort if zero tokens found.
        if ( numTokens == 0 )
        {
            continue;
        }
        // Exit if input is "quit"
        if ( strcasecmp(line, "quit") == 0 )
        {
            break;
        }

        args[i] = NULL;
        ofile = args[i-1];
        printf("%s\n", ofile);
        struct thread_data thread_data_array[i];
        pthread_t threads[i];

        for(t=0; t<i; t++)
        {
            thread_data_array[t].thread_id = t;
            thread_data_array[t].message = args[t];

            rc = pthread_create(&threads[t], NULL, PrintHello, (void *)&thread_data_array[t]);
            if (rc)
            {
                printf("ERROR; return code from pthread_create() is %d\n", rc);
                exit(-1);
            }
        }

        // Wait for threads to complete work.
        for(t=0; t<i; t++)
        {
            pthread_join(threads[t], NULL);
        }
    }      
}

---

Sample Run

---

% 
% 
% 
% Hello world. This is a test
test
Hello World! It's me, thread test!
Hello World! It's me, thread a!
Hello World! It's me, thread is!
Hello World! It's me, thread This!
Hello World! It's me, thread world.!
Hello World! It's me, thread Hello!
% 
% 1 2 3
3
Hello World! It's me, thread 3!
Hello World! It's me, thread 2!
Hello World! It's me, thread 1!
% QUit