Thread synchronization problem: possible race, misuse of volatility, cache consistency?

Question

I am familiar with pthreads programming; The following code is a simple producer / consumer design where data is placed / retrieved from a global list. The problem is that the data pointer in the user function is trying to be freed twice. Also, if I add printf() at the beginning of the loop, everything will be fine ... What am I doing wrong? I suspect the misuse of the volatile keyword or something hidden by the cache ... If this is not a design problem (probably this: p).

Thanks for your ideas.

Note. malloc()/free() is thread safe on my system. I am compiling with $ gcc -pthread -O0 , which should, I think, reduce possible design errors due to misuse of volatile . Finally, we do not care about this piece of code with a lack of memory (in the case of more manufacturers than the consumer).

Edit: The code for one chapter of the list has been changed.

 #include <pthread.h> #include <stdlib.h> #include <string.h> pthread_mutex_t lock; pthread_cond_t new_data; struct data { int i; struct data *next; }; struct data *list_head = NULL; void *consume(void *arg) { struct data *data; while (1) { pthread_mutex_lock(&lock); while (list_head == NULL) { pthread_cond_wait(&new_data, &lock); } data = list_head; list_head = list_head->next; pthread_mutex_unlock(&lock); free(data); } return NULL; } void *produce(void *arg) { struct data *data; while (1) { data = malloc(sizeof(struct data)); pthread_mutex_lock(&lock); data->next = list_head; list_head = data; pthread_mutex_unlock(&lock); pthread_cond_signal(&new_data); } return NULL; } int main() { pthread_t tid[2]; int i; pthread_mutex_init(&lock, NULL); pthread_cond_init(&new_data, NULL); pthread_create(&tid[0], NULL, consume, NULL); pthread_create(&tid[1], NULL, produce, NULL); for (i = 0; i < 2; i++) { pthread_join(tid[i], NULL); } } 

And the conclusion:

 $ ./a.out *** glibc detected *** ./a.out: double free or corruption (fasttop): 0x00007f5870109000 *** 

Answer 1

Consider the following scenario:

• to produce → the acquired castle
• consume → block waiting
• to produce → select d0, write_ptr = d0, read_ptr = d0, signal, unlock
• consume → acquired lock
• produce → lock wait
• consume → satisfied condition
• consume -> data = d0, read_ptr = NULL, unlock
• consume → free d0
• make → the acquired lock, highlight d1
• consume → block waiting
• write_ptr->next = d1 write_ptr != null so write_ptr->next = d1
• read_ptr == null so read_ptr = d1

Follow step 11. write_ptr is still d0, although it was free regardless of the producer thread. You must ensure that consume does not invalidate write_ptr .

A doubly linked list will allow you to avoid some of these difficulties (since readers and writers work from different ends).

Main:

• Create TAIL HEAD and TAIL , link HEAD and TAIL
• Breed producer
• Spawn consumer

Manufacturer:

• Lock
• Create node
• Link HEAD->next->prev to node
• Link node->next to HEAD->next
• HEAD->next link to node
• Link node->prev to HEAD
• Open
• Signal

Consumer:

• Lock
• Wait TAIL->prev != HEAD ( do { pthread_cond_wait } while (condition); )
• data = TAIL->prev
• Link TAIL->prev to data->prev
• TAIL->prev->next TAIL to TAIL
• Open
• Use data

Answer 2

I believe your problem is in the following lines:

 if (NULL != write_ptr) write_ptr->next = data; write_ptr = data; if (NULL == read_ptr) read_ptr = data; 

I don’t think you are building your list correctly. In fact, I do not understand why you have two lists. But anyway...

I assume that you want to add your new data to the top of the list. Otherwise, you will need a pointer to the tail, or you will need to chase to the end of the list each time.

To do this, you need to add the current list as the next data item. It should look like this:

 data->next = write_ptr; write_ptr = data; 

There is no need to check for NULL on write_ptr.

Answer 3

Update. As Billy Onel pointed out, pthread functions provide the necessary memory barriers, so there is no need to declare anything mutable if it is protected by a pthread lock. (See this question for details: Does the variable protect with mutex pthread, is it also not cached? )

But I got another explanation for some odd behavior: your linked list, which the producer creates, is broken: suppose write_ptr not NULL and is observing the behavior:

 /* 1 */ if (NULL != write_ptr) /* 2 */ write_ptr->next = data; /* 3 */ write_ptr = data; 

Say write_ptr points to a previously highlighted AAnext instance - NULL. We recently allocated an instance of B and set everything to NULL (therefore: B.next - NULL).

Line 1 evaluates to true, so line 2 is executed. A.next now points to B. After line 3 completes, write_ptr points to B B.next - NULL => A is lost, resulting in a memory leak.

But for now, I don’t understand why libc complains about double freedom.

Answer 4

Line error

 if (NULL != write_ptr) write_ptr->next = data; write_ptr = data; 

This should read: if (NULL! = Write_ptr) data-> next = write_ptr; write_ptr = data;

For debugging purposes, also ensure that you get the expected values ​​from the queue.

There is also no need for volatile variables. since your queue is protected by mutexes, the operating system ensures that access to the queue is atomic. volatile is only required when accessing memory-mapped hardware resources and should never be used for synchronization. All he does is force the deletion of data into memory.

There is one more problem. If your producer is faster than your consumer, in the end you will run out of memory unless you limit the size of the queue and make the producer wait for the consumer. Smaller queues are more responsive, and the only reason for large queues is to smooth out producer disturbances.