[Spice-devel] [PATCH] Use RING_FOREACH_SAFE in red_channel.c functions which are missing it

[Christophe Fergeau]

On Fri, Jul 05, 2013 at 05:11:46PM +1000, David Gibson wrote:
> Currently, both red_channel_pipes_add_type() and
> red_channel_pipes_add_empty_msg() use plaing RING_FOREACH() which is not
> safe versus removals from the ring within the loop body.
> 
> Although it's rare, such a removal can occur in both cases.  In the case
> of red_channel_pipes_add_type() we have:
>     red_channel_pipes_add_type()
>     -> red_channel_client_pipe_add_type()
>         -> red_channel_client_push()
> 
> And in the case of red_channel_client_pipes_add_empty_msg() we have:
>     red_channel_client_pipes_add_empty_msg()
>     -> red_channel_client_pipe_add_empty_msg()
>         -> red_channel_client_push()
> 
> But red_channel_client_push() can cause a removal from the clients ring if
> a network error occurs:
>     red_channel_client_push()
>     -> red_channel_client_send()
>         -> red_peer_handle_outgoing()
>             -> handler->cb->on_error callback
>             =  red_channel_client_default_peer_on_error()
>                 -> red_channel_client_disconnect()
>                     -> red_channel_remove_client()
>                         -> ring_remove()
> 
> When this error path does occur, the assertion in RING_FOREACH()'s
> ring_next() trips, and the process containing the spice server is aborted.
> i.e. your whole VM dies, as a result of an unfortunately timed network
> error on the spice channel.

Looks good to me, thanks for tracking it down! I'll let other people more
familiar with spice give a final ACK

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130705/eb59c149/attachment-0001.pgp>