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

[David Gibson]

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);
-- 
1.8.1.4

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- 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/f29fb662/attachment.pgp>