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

[Jiri Denemark]

On Fri, May 22, 2015 at 13:48:16 -0400, John Ferlan wrote:
> 
> 
> 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?

This would be a serious bug in the compiler. The function also gets the
queue pointer, which means the compiler should not consider queue->head
was unchanged.

Jirka