Two threads somehow using the same data (seg fault with pthreads in c)

I have a (fairly large) program that encodes a bitmap image to a compressed format. My goal is to thread this program to encode multiple bitmap images to multiple compressed images. All the variables are "standalone," meaning I should only have to tell each thread, "Input file name is..., output file name is..., go..." then wait with a pthread_join at the end of my main.

The code works as non-threaded, and also works with one thread. My current test is on two threads. Most times I get a segmentation fault. Looking at the call stack using gdb, it happens at various places throughout the program. Sometimes (once or twice), it's gone all the way through and completed both threads successfully. When I looked at the output files, however, they were the same. Both threads compressed the same image. I don't understand how they are both using the same data, as each thread gets a different file name and should open a different image, using different variables to hold that data, etc. etc.

Does anyone have any idea what might be causing this? I'm assuming the seg faults are caused by the threads switching back and forth and somehow writing/reading some data that the other thread was writing/reading, but they should be operating on completely separate data. I'm fairly new to Linux threads, but have used Windows threads a decent amount.

Thanks for any help...the code I'm using to create the threads and their start routine is below.

int main()
{
    int i;
    pthread_t thread1;
    pthread_t thread2;

    char ** args1;
    char ** args2;
    for(i = 0; i < 2; i++)
    {
        args1 = calloc(1, sizeof(char)*MAX_STRING);
        args2 = calloc(1, sizeof(char)*MAX_STRING);
    }

    args1[0] = "tile0_0.bmp";
    args1[1] = "tile0_0.jp2";

    args2[0] = "tile0_1.bmp";
    args2[1] = "tile0_1.jp2";

    if( pthread_create( &thread1,
                        NULL, //NULL = default attr
                        start_routine,
                        (void)*args1 ) != 0 )
    {
        printf("error creating thread1\n");
    }

    if( pthread_create( &thread2,
                        NULL,
                        start_routine,
                        (void)*args2 ) != 0 )
    {
        printf("error creating thread2\n");
    }
...
}

static void * start_routine(void * arg)
{
    char ** myargs = (char **)arg;
    char * filename = (char*)calloc(1, sizeof(char)*64);
    char * outfilename = (char*)calloc(1, sizeof(char)*64);

    filename = myargs[0];
    outfilename = myargs[1];

    printf("myargs[0] (filename): %s\n", myargs[0]);
    printf("myargs[1] (outfile): %s\n", myargs[1]);

....
}

I think that your problem is with the myargs array. I think that you have to create a new copy of myargs with malloc or calloc each time that you enter start_routine. Initialize each new copy of myargs from arg as you are doing now.

Free the myargs memory each time you exit start_routine or you will be creating a memory leak. You also should free the args1 and args2 memory when the corresponding thread finishes to avoid memory leaks. Memory leaks are a trivial problem in this code but when you generalize the code they could become significant.

------------------
Steve Stites

Now that you pointed it out, that makes a lot of sense. Thanks for the advice. Hopefully this is the reason for the seg faults as well. The machine I have the code on is at work, so I'll get back tomorrow and let you know if that worked.

About the memory leaks, I am freeing the malloc/calloc'd variables, but didn't show that code.

I noticed that I was allocating memory in a for loop without even using the control variable, and I wasn't allocating space to the char** before mallocing in that for loop. So I fixed that, added the malloc to myargs, and reran the program. I seem to be able to complete successfully more often, but I still get seg faults most of the time.

Here's another weird one: one of the times it completed successfully, it compressed the same image. Another time, it compressed both images, but tile0_0.bmp was compressed to tile0_1.jp2, and tile0_1.bmp used tile0_0.jp2 as the output.

Updated code below.

int main()
{
    int i;
    pthread_t thread1;
    pthread_t thread2;

    char ** args1 = calloc(1, sizeof(char*)*2);
    char ** args2 = calloc(1, sizeof(char*)*2);
    for(i = 0; i < 2; i++)
    {
        args1[i] = calloc(1, sizeof(char)*MAX_STRING);
        args2[i] = calloc(1, sizeof(char)*MAX_STRING);
    }

    args1[0] = "tile0_0.bmp";
    args1[1] = "tile0_0.jp2";

    args2[0] = "tile0_1.bmp";
    args2[1] = "tile0_1.jp2";

    if( pthread_create( &thread1,
                        NULL, //NULL = default attr
                        start_routine,
                        (void)*args1 ) != 0 )
    {
        printf("error creating thread1\n");
    }

    if( pthread_create( &thread2,
                        NULL,
                        start_routine,
                        (void)*args2 ) != 0 )
    {
        printf("error creating thread2\n");
    }
...
    for(i = 0; i < 2; i++)
    {
        free(args1[i]);
        free(args2[i]);
    }
    free(args1);
    free(args2);

    return 0;
}

static void * start_routine(void * arg)
{
    int i;
    char ** myargs = calloc(1, sizeof(char*)*2);
    for(i = 0; i < 2; i++)
    {
        myargs[i] = calloc(1, sizeof(char)*MAX_STRING);
    }
    myargs = (char **)arg;
    char * filename = (char*)calloc(1, sizeof(char)*64);
    char * outfilename = (char*)calloc(1, sizeof(char)*64);

    filename = myargs[0];
    outfilename = myargs[1];

    printf("myargs[0] (filename): %s\n", myargs[0]);
    printf("myargs[1] (outfile): %s\n", myargs[1]);

....
    for(i = 0; i < 2; i++)
    {
        free(myargs1[i]);
    }
    free(myargs);
}

I guess the main problem here is that you allocate char memory, and then initialize them with string literals (basically overwriting the pointers of the newly allocated stuff with other pointers) and then freeing the literals (which the system takes care of), resulting in memory errors.

In short, for literals you don't need to allocate extra memory (needless to say no need to free it afterwards).

Try it like that (untested, changes surrounded by #####):

int main()
{
    int i;
    pthread_t thread1;
    pthread_t thread2;

    char ** args1 = calloc(1, sizeof(char*)*2);
    char ** args2 = calloc(1, sizeof(char*)*2);
    #####
    /*for(i = 0; i < 2; i++)
    {
        args1[i] = calloc(1, sizeof(char)*MAX_STRING);
        args2[i] = calloc(1, sizeof(char)*MAX_STRING);
    }*/
    #####

    args1[0] = "tile0_0.bmp";
    args1[1] = "tile0_0.jp2";

    args2[0] = "tile0_1.bmp";
    args2[1] = "tile0_1.jp2";

    if( pthread_create( &thread1,
                        NULL, //NULL = default attr
                        start_routine,
                        (void)*args1 ) != 0 )
    {
        printf("error creating thread1\n");
    }

    if( pthread_create( &thread2,
                        NULL,
                        start_routine,
                        (void)*args2 ) != 0 )
    {
        printf("error creating thread2\n");
    }
...
    #####
    /*for(i = 0; i < 2; i++)
    {
        free(args1[i]);
        free(args2[i]);
    }*/
    #####
    free(args1);
    free(args2);

    return 0;
}

static void * start_routine(void * arg)
{
    #####
    /*int i;
    char ** myargs = calloc(1, sizeof(char*)*2);
    for(i = 0; i < 2; i++)
    {
        myargs[i] = calloc(1, sizeof(char)*MAX_STRING);
    }
    myargs = (char **)arg;
    char * filename = (char*)calloc(1, sizeof(char)*64);
    char * outfilename = (char*)calloc(1, sizeof(char)*64);

    filename = myargs[0];
    outfilename = myargs[1];*/
    

    char ** myargs = (char **)arg;

    char * filename = myargs[0];
    char * outfilename = myargs[1];
    #####

    printf("myargs[0] (filename): %s\n", myargs[0]);
    printf("myargs[1] (outfile): %s\n", myargs[1]);

....
    #####
    /*for(i = 0; i < 2; i++)
    {
        free(myargs1[i]);
    }*/
    #####
    free(myargs);
}

args1[0] = strdup("tile0_0.bmp");

ta0kira

Tried fantas' and ta0kira's suggestions, but I'm still getting the same issue. I really don't think the issue is in the assignment of the strings, because for the multiple ways I've assigned them, they print out correctly as each thread starts up.

I was talking with a few other people, and the only thing we could come up with that was possibly the problem is that the threads are allocating memory in the same place as they're switching back and forth. Are malloc and calloc thread safe? Is it possible for one thread to begin to allocate, go to sleep, the other thread to allocate its space, go to sleep, and then the first thread allocates over portions of the second thread's space?

We figured that this could cause the seg faults and could explain the program encoding opposite images.

Thanks for your help

I oversaw something in my edited code: The last "free(myargs);" needs to go as well (i.e. as in comment it out, in the second last line), as it would lead to double freeing of the same memory (originally allocated by the main thread).
Otherwise the code looks OK-ish, assuming that the parts that were left out with a "..." are OK too.

What's this all about?

(void)*args1

That doesn't look safe, or even like something that should compile. Try this:

(void*) args1

ta0kira

Originally Posted by fortenbt Tried fantas' and ta0kira's suggestions, but I'm still getting the same issue. I really don't think the issue is in the assignment of the strings, because for the multiple ways I've assigned them, they print out correctly as each thread starts up.

Without using dynamic allocation (i.e. duplicating the string literals,) this is just a coincidence.
ta0kira