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

[Yonit Halperin]

Ack. Thanks! We have recently associated this problem with some opened 
bugs we have. I believe Uri is working on a patch for a similar fix to 
the red_pipes.* routines in red_worker.


On 07/05/2013 03:11 AM, 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.
>
> Please apply.
>
> Signed-off-by: David Gibson <dgibson at redhat.com>
> ---
>   server/red_channel.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/server/red_channel.c b/server/red_channel.c
> index c0b1781..8742008 100644
> --- a/server/red_channel.c
> +++ b/server/red_channel.c
> @@ -1572,9 +1572,9 @@ void red_channel_client_pipe_add_type(RedChannelClient *rcc, int pipe_item_type)
>
>   void red_channel_pipes_add_type(RedChannel *channel, int pipe_item_type)
>   {
> -    RingItem *link;
> +    RingItem *link, *next;
>
> -    RING_FOREACH(link, &channel->clients) {
> +    RING_FOREACH_SAFE(link, next, &channel->clients) {
>           red_channel_client_pipe_add_type(
>               SPICE_CONTAINEROF(link, RedChannelClient, channel_link),
>               pipe_item_type);
> @@ -1593,9 +1593,9 @@ void red_channel_client_pipe_add_empty_msg(RedChannelClient *rcc, int msg_type)
>
>   void red_channel_pipes_add_empty_msg(RedChannel *channel, int msg_type)
>   {
> -    RingItem *link;
> +    RingItem *link, *next;
>
> -    RING_FOREACH(link, &channel->clients) {
> +    RING_FOREACH_SAFE(link, next, &channel->clients) {
>           red_channel_client_pipe_add_empty_msg(
>               SPICE_CONTAINEROF(link, RedChannelClient, channel_link),
>               msg_type);
>
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>