Help:thread is corrupting global data

[champnim]

I am facing a strange problem. I have a multi-threaded C program that is communicating with the COM port. I have a dedicated thread to read from the port that remains in an infinite loop to read any incoming data. Whenever something is read my 'read thread' creates a new entry in a global linked list, specifying a code for the thing read as well as the string read itself. When the main thread realises that an entry has been made into the list, it accesses the linked list & after reading the data stored there, it deletes the entry.
Here is my code:

//Code for Linked List 
struct fifo
{
    int result;                                                       
    char *info;
    struct fifo *next;
}*node;

void add(struct fifo **n,int data,char *text)
{
    struct fifo *temp,*node1;
    if (*n==NULL)
    {
        temp=(struct fifo *)malloc(sizeof(struct fifo));
        printf("%d is to be stored\n",data);
        temp->result=data;
        printf("%d is stored\n",temp->result);
        temp->info=(char *)malloc(strlen(text)+1);
        strcpy(temp->info,text);
        temp->next=NULL;
        *n=temp;
    }
    else
    {
        temp=*n;
        while(temp->next!=NULL)
            temp=temp->next;
        node1=(struct fifo *)malloc(sizeof(struct fifo));
        node1->result=data;
        node1->info=(char *)malloc(strlen(text)+1);
        strcpy(node1->info,text);
        node1->next=NULL;
        temp->next=node1;
    }
}

void del(struct fifo **n)
{
    struct fifo *temp;
    if (*n==NULL)
    {
        printf("Linked list is empty\n");
    }
    else
    {
        temp=*n;
        *n=temp->next;
    }
}
//Code for Linked List ends


//Inside read thread that will add to the linked list
            
      //If a particular condition is satisfied

               EnterCriticalSection(&critical);
               add(&node,50,location);          //location holds string read
               LeaveCriticalSection(&critical);    //exit condition after this

//End of read thread


//Inside main thread that reads & deletes from the linked list

InitializeCriticalSection(&critical);

while(1)
{    
     if (node==NULL)                             //idle case
    {
        printf("Nothing to read\n");
        Sleep(4000);
    }
    else                               //Linked List is not empty
    {
        EnterCriticalSection(&critical);

        temp=(struct fifo *)malloc(sizeof(struct fifo));   //Read node
        printf("%d\n%s\n",node->result,node->info);       
        op=node->result;
        printf("op is %d\n",op);
        detail=(char *) malloc (strlen(node->info));
        strcpy(detail,node->info);
        printf("%s\n",detail);
        del(&node);                               //delete node

        LeaveCriticalSection(&critical);

        if (node==NULL)                        //Check condition to confirm  
            printf("node is null\n");          //that node has been deleted
        else                     
            printf("node is not null\n");

        if (op==50)                  //op has been read from the node
        {
            //call function with detail as parameter...call is successful
            free(detail);

            Sleep(2000);
            //Make some other function calls

            op=-1;

            if (node==NULL)                 //Another check condition to 
                    printf("%d\n",op);      //confirm that node is empty
            else                
                    printf("node is not null\n");
            
        }
        //some other similar conditions 
}

DeleteCriticalSection(&critical);
//Close threads

return (0);
//End of main//Code for Linked List 
struct fifo
{
    int result;                                                       
    char *info;
    struct fifo *next;
}*node;

void add(struct fifo **n,int data,char *text)
{
    struct fifo *temp,*node1;
    if (*n==NULL)
    {
        temp=(struct fifo *)malloc(sizeof(struct fifo));
        printf("%d is to be stored\n",data);
        temp->result=data;
        printf("%d is stored\n",temp->result);
        temp->info=(char *)malloc(strlen(text)+1);
        strcpy(temp->info,text);
        temp->next=NULL;
        *n=temp;
    }
    else
    {
        temp=*n;
        while(temp->next!=NULL)
            temp=temp->next;
        node1=(struct fifo *)malloc(sizeof(struct fifo));
        node1->result=data;
        node1->info=(char *)malloc(strlen(text)+1);
        strcpy(node1->info,text);
        node1->next=NULL;
        temp->next=node1;
    }
}

void del(struct fifo **n)
{
    struct fifo *temp;
    if (*n==NULL)
    {
        printf("Linked list is empty\n");
    }
    else
    {
        temp=*n;
        *n=temp->next;
    }
}
//Code for Linked List ends


//Inside read thread that will add to the linked list
            
      //If a particular condition is satisfied

               EnterCriticalSection(&critical);
               add(&node,50,location);          //location holds string read
               LeaveCriticalSection(&critical);    //exit condition after this

//End of read thread


//Inside main thread that reads & deletes from the linked list

InitializeCriticalSection(&critical);

while(1)
{    
     if (node==NULL)                             //idle case
    {
        printf("Nothing to read\n");
        Sleep(4000);
    }
    else                               //Linked List is not empty
    {
        EnterCriticalSection(&critical);

        temp=(struct fifo *)malloc(sizeof(struct fifo));   //Read node
        printf("%d\n%s\n",node->result,node->info);       
        op=node->result;
        printf("op is %d\n",op);
        detail=(char *) malloc (strlen(node->info));
        strcpy(detail,node->info);
        printf("%s\n",detail);
        del(&node);                               //delete node

        LeaveCriticalSection(&critical);

        if (node==NULL)                        //Check condition to confirm  
            printf("node is null\n");          //that node has been deleted
        else                     
            printf("node is not null\n");

        if (op==50)                  //op has been read from the node
        {
            //call function with detail as parameter...call is successful
            free(detail);

            Sleep(2000);
            //Make some other function calls

            op=-1;

            if (node==NULL)                 //Another check condition to 
                    printf("%d\n",op);      //confirm that node is empty
            else                
                    printf("node is not null\n");
            
        }
        //some other similar conditions 
}

DeleteCriticalSection(&critical);
//Close threads

return (0);
//End of main

Now here's the output:

Nothing to read
//after some time
50         //value of int stored in node
25         //string stored in node

op is 50                 
25

node is null

//results of function calls successfully printed

node is not null
Nothing to read
//after some time
50         //value of int stored in node
25         //string stored in node

op is 50                 
25

node is null

//results of function calls successfully printed

node is not null

As you can see from the output, 'main' first showed that the node is null, but after the function calls, it said that the node is not null. The function calls being made involve issuing commands to the port & reading their response(no entry is made to the linked list during this read).
I have noticed that I am getting the above output in situations when I have to read a very large string from the port. In such situations, the thread does not read the entire response in one attempt, but instead does so in multiple attempts. As a result, my linked list pointer somehow gets modified & hence it does not point to null anymore.
However, whenever the response has been small enough to be read in a single attempt, I have got no such error.
Can anybody please help me........

[kirants]

You need synchronization. Please read the articles here:
Arjay's multithreading articles

[champnim]

I don't think the problem has got anything to do with synchronization. like I said, my program is working fine for all cases except when the input to be read from the port exceeds 256 bytes. I think the problem has something to do with buffer overflow. Can anybody help me?

[Arjay]

Is the data that arrives from the serial port always null terminated?

If it's not, you are going to have a problem when you call strlen().

[champnim]

Yes the data is null terminated. I am able to read the entire data, irrespective of its length. However I am unable to take any action on other incoming data, since the address of the node of the global linked list gets lost after reading. So, instead of being able to read incoming data indefinitely, my read thread can only read once(when incoming data is > 256 bytes). please help...

[Codeplug]

As kirants pointed out, you need synchronization.

In your code, "node" looks like the "global linked list" header. Since this variable is accessed (read/write) by multiple threads, you must synchronize the access to that variable.

In your posted code, there several spots where you access "node" without first acquiring the critical section.

I would also get rid of all those Sleep calls and use either a semaphore (which has an associated resource count) or an Event (which doesn't) to "signal" the main thread that data is available in the list. This is where functions like WaitForSingleObjec() come into play (check out Arjay's signature).

Fix up your synchronization. If you still have problems/questions, [re]post relevant code.

gg

[champnim]

I'm not sure I have understood why synchronization is causing the problems that I have been facing. But, following the suggestions of people on this forum, I have got down to doing it. I have a query about it though. Isn't WaitForSingleObject like a blocking function, in the sense that while it is waiting for the event, my thread will be unable to perform any activity. I want to have a scenario where the main thread is doing its work & the moment a node is added, I want my main thread to be signalled, and, if possible, stop its current execution & proceed to read the list. Is it possible to do so using Wait?
For that matter, is it possible to have a sort of timer interrupt wherein, the main thread will be informed of an addition to the list after a certain time delay & not immediately?

[JohnW@Wessex]

One method that can alleviate synchronisation problems is to make just one thread responsible for adding to and deleting from the data structure. Once the main thread has read the data then it raises a 'data has been read' event, which is picked up by the WaitForMultipleObjects in the COM reader thread.

[Codeplug]

>> Isn't WaitForSingleObject like a blocking function
Yes. And it also has a "timeout" parameter. So for instance, if you just wanted to check if an event was signaled (without blocking), you use a timeout of 0.

>> I want to have a scenario where the main thread is doing its work & the moment a node is added, I want my main thread to be signaled, and, if possible, stop its current execution & proceed to read the list.
This can be done, in a sense. You don't really "stop execution". The worker thread adds stuff to the list, then "signals" a waitable object (event, semaphore, etc). It's then up to the main thread to check on that object's status at some point and act on it if it's signaled.

>> is it possible to have ... the main thread ... informed of an addition to the list after a certain time delay & not immediately?
So now we know it's up to the main thread to check on the waitable object's status - so the you could simply check the object on a periodic basis by measuring elapsed time. In most cases however, it's better to process the data as soon as possible.

gg

[Arjay]

Kirant mentioned my multi-threading articles. In addition to these, see the first link listed in my signature line (you'll have to turn on signatures in your UserCP to see them).

This link is a sample that uses a std::queue to pass data between a secondary thread and insert the item into a list view of the UI thread. The queue is shared and synchronized between threads and the secondary thread signals the UI via a message when an item as been added to the queue.

Edit: Actually if I recall correctly there are two secondary threads adding items to the queue, and the UI thread is popping the items off the queue and putting them into list control.