Thread: Segmentation fault - threading

Hello,

I would like to ask to my code, when I'm trying inserting to the linked list new nodes through threads, after first insert(by first thread) I've got:

Code:

Segmentation fault (core dumped)

here is my code:

Code:

#include <stdio.h>
#include <string.h>
#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
#include <time.h>
typedef struct list {
        int number;
        struct list *next;
}LIST;
LIST *a;
void *init()
{
        a = (LIST*)malloc(sizeof(LIST));
        a->number = -1;
        a->next = NULL;


        return a;
}
void *insert(int number)
{
        LIST *pom;
        LIST *new;


        new = (LIST*)malloc(sizeof(LIST));
        new->number = number;
        new->next = NULL;


        if(a->number == -1 || a->number>=number)
        {
                if(a->number == -1)
                {
                        a->number=number;
                        return a;
                }
                new->next=a;
                a=new;
                return;
        }
        else
        {
                pom=a;
                while(pom->next != NULL && pom->next->number < number)
                {
                        pom=pom->next;
                }
                new->next=pom->next;
                pom->next=new;
        }
}
void *delete(int number)
{
        LIST *prev=NULL;
        LIST *curr;


        curr=a;


        while(1)
        {
                if(number == curr->number)
                {
                        if(prev == NULL)
                        {
                                a=curr->next;
                                free(curr);
                        }
                        prev->next=curr->next;
                        free(curr);
                }
                prev=curr;
                curr=curr->next;
        }
}
void *thread(void *arg)
{
        int *pole;
        int i, randCount;
        randCount = (int*)arg;
        pthread_mutex_t mutex1 = PTHREAD_MUTEX_INITIALIZER;
        pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER;


        pole = malloc(randCount*sizeof(int));
        pthread_t id = pthread_self();


        for(i=0; i<randCount; i++)
        {
                pole[i]=rand() % 20;
pthread_mutex_lock( &mutex1 );
                insert(pole[i]);
pthread_mutex_unlock( &mutex1 );
                printf("Vlozene: %d by: %d\n", pole[i], id);
        }


        for(i=0; i<randCount; i++)
        {
pthread_mutex_lock( &mutex2 );
                delete(pole[i]);
pthread_mutex_unlock( &mutex2 );


                printf("Vymazane: %d by: %d\n", pole[i], id);
        }


        printf("\n");
        return NULL;
}
void main(int argc, char** argv)
{
        LIST *pom=NULL;
        int numbersCount, threadsCount, i;
        printf("Zadajte pocet cisiel a pocet vlakien:\n");
        scanf("%d %d", &numbersCount, &threadsCount);
        int *threadsArray, result;


        if(numbersCount < threadsCount)
        {
                printf("Pocet vlakien je vacsi ako pocet cisiel! Pocet vlakien ");
                printf("bude upraveny na hodnotu, ktora sa rovna poctu cisiel.\n");
                threadsCount = numbersCount;
        }


        a = init();


        threadsArray = malloc(threadsCount*sizeof(int));
        result = numbersCount / threadsCount;
        for(i=0; i<threadsCount; i++)
        {
                threadsArray[i]=result;
        }
        result = numbersCount % threadsCount;
        for(i=0; i<result; i++)
        {
                threadsArray[i]++;
        }


        for(i=0; i<threadsCount; i++)
        {
                printf("%d ",threadsArray[i]);
        }


        //threading
        int err;
        pthread_t pth;


        for(i=0; i<threadsCount; i++)
        {
                err = pthread_create(&pth, NULL, &thread, threadsArray[i]);


                if(err != 0)
                        printf("\ncan't create thread :[%s]", strerror(err));
                else
                        printf("\n Thread created successfully\n");
        sleep(1);
        }
        for(i=0; i<threadsCount; i++)
        {
                pthread_join(pth, NULL);
        }


}

when I tried run without threading, insert worked fine.


Thank you

First and foremost:
* Use CONSISTENT indentation and spacing. Group statements together that have relevance together
* DO NOT cast the return of malloc! All this does is hide bugs in your program, in C when your compiling is squawking at you it's generally not a good idea to "fix" it by adding a CAST. Please make sure you include stdlib.h and do not cast the return, it will make your code easier to read and easier to modify.

That being said, your delete() function has some serious issues. Let's examine it more closely:

Code:

void *delete(int number)
{
        LIST *prev=NULL;
        LIST *curr;

        curr=a;
        while(1) {

Already we are seeing some issues here, you should assign a immediately upon initialization of curr, instead of declaring it and then initializing it in two separate statements. Secondly, why are you looping infinitely? You realize this loop has no exit points what-soever? IE this is an infinite loop so I have no idea how you claim that this is running "correctly" without threading, that is simply not true. If we fix these 2 problems, that quickly brings a big improvement:

Code:

void *delete(int number)
{
        LIST *prev=NULL;
        LIST *curr = a;  /* Assign our curr pointer to the first element of the list */

        while(curr) {  /* Continue searching while we have a valid pointer (not NULL) */

Ok so now we fixed the problem of infinite looping. As we look further down, we see some further issues that need addressing:

Code:

        { /* Loop body start */
                if(number == curr->number)      // Ok  keep checking to see if we found our target number
                {
                        if(prev == NULL)               // Check "special" case when we are deleting the root (first) element in the list
                        {
                                a=curr->next;
                                free(curr);                // All fine
                        } // !!UH-OH!! We just checked if prev could be NULL yet we then try to dereference it anyway, this should be protected by an else condition!! This is undefined behavior/memory access violation
                        prev->next=curr->next;   
                        free(curr);  // !!UH-OH!! Duplicate free call!! again, this should be inside an else condition, as this OR the code in the if condition above should be executed only. NOT both.
                        /* No break; here even though we found the element. We continue to code below, causing undefined behavior again! */
                }
/* Add some spacing here to separate the body of the loop from the loop related functions */
                prev=curr;
                curr=curr->next; /* So now curr becomes NULL when we reach the end of the list ENDing our loop, using the proper while(curr) condition*/
        }

So now there are some serious memory access situations here. You repeatedly try to double free values and/or de-reference invalid pointers, invoking undefined behavior and the most likely cause of the segfaults you are seeing.

Fixing this is easy, you simply need to protect those statements with an else branch, AND you need to break; out of the loop once you have free'd the 'curr' pointer because it is no longer valid. I will leave making those changes to you as you should be more than capable.

One last thing, I have no idea what you are doing with some of the for () loops you have in main() but I'm sure there are more bugs in that routine as well but unrelated to the segfaulting.

The thread function has a race condition. Calls of insert() are protected by mutex1. Calls of delete() are protected by mutex2. These is nothing stopping thread A getting past the calls of insert() and calling delete() while thread B is busily calling insert(). Since insert() and delete() access the same globals (a and the nodes it links to) effectively that means access to those globals are not synchronised.

Also mutex1 and mutex2 are local to the thread function, and are not statics. PTHREAD_MUTEX_INITIALIZER is also not really guaranteed to work to initialise non-statics (although that does depend on version of the posix specification your implementation complies with).

Just wrapping access to shared data in any old mutex doesn't work. The threads need to explicitly cooperate with the help of mutexes, and the code here is not ding that.