[libvirt] [PATCH 01/10] util: Introduce thread queues

[John Ferlan]

On 05/22/2015 10:31 AM, Jiri Denemark wrote:
> On Fri, May 22, 2015 at 10:16:26 -0400, John Ferlan wrote:
>>
>>
>> On 05/21/2015 06:42 PM, Jiri Denemark wrote:
>>> Our usage of pthread conditions does not allow a single thread to wait
>>> for several events from different sources. This is because the condition
>>> is bound to the source of the event. We can invert the usage by giving
>>> each thread its own condition and providing APIs for registering this
>>> thread condition with several sources. Each of the sources can then
>>> signal the thread condition.
>>>
>>> Thread queues also support several threads to be registered with a
>>> single event source, which can either wakeup all waiting threads or just
>>> the first one.
>>>
>>> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
>>> ---
>>>  po/POTFILES.in            |   1 +
>>>  src/Makefile.am           |   2 +
>>>  src/libvirt_private.syms  |  15 ++
>>>  src/util/virthreadqueue.c | 343 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  src/util/virthreadqueue.h |  54 ++++++++
>>>  5 files changed, 415 insertions(+)
>>>  create mode 100644 src/util/virthreadqueue.c
>>>  create mode 100644 src/util/virthreadqueue.h
>>>
>>
>> Ran the series through Coverity and came back with this gem
>>
>>> +
>>> +void
>>> +virThreadQueueFree(virThreadQueuePtr queue)
>>> +{
>>> +    if (!queue)
>>> +        return;
>>> +
>>> +    while (queue->head)
>>> +        virThreadQueueRemove(queue, queue->head);
>>
>> (3) Event cond_true: 	Condition "queue->head", taking true branch
>> (6) Event loop_begin: 	Jumped back to beginning of loop
>> (7) Event cond_true: 	Condition "queue->head", taking true branch
>>
>> 283  	    while (queue->head)
>>
>> (4) Event freed_arg: 	"virThreadQueueRemove" frees "queue->head". [details]
>> (5) Event loop: 	Jumping back to the beginning of the loop
>> (8) Event deref_arg: 	Calling "virThreadQueueRemove" dereferences freed pointer "queue->head". [details]
>>
>> 284  	        virThreadQueueRemove(queue, queue->head);
> 
> False positive. If virThreadQueueRemove frees queue->head, it also
> updates it with queue->head->next.
> 

I understand and looked at the code, but I think this is a case where
pass by reference and pass by value makes a difference.  Upon return
what causes queue->head to be evaluated again?  The call passes
queue->head by value and removes it from queue.  Upon return nothing
causes queue->head to be evaluated again thus wouldn't we enter the loop
the second time with the same address?


John