[Spice-devel] [server PATCH 0/8] use a SAFE version of RING_FOREACH (worker/channel)

[Yonit Halperin]

Ack Series,
with the comment that the actual risky places that cause the crash, may 
better use safe red_channel.c routines instead. But it can be done later.

On 07/08/2013 06:32 AM, Uri Lublin wrote:
> If in RING_FOREACH (in == somewhere in the callee-tree) there is
> a call to read or write, it is not safe, and RING_FOREACH_SAFE
> should be used.
>
> A call to such read/write that fails, may lead
> to disconnecting from the client, which means
> cleaning up things, and possibly removing from the ring
> the specific node (channel/client) that is currently
> being iterated by the FOREACH loop.
>
> When the FOREACH loop tries to get the next item, spice-server
> asserts and aborts in ring_next():
>   -- assertion `pos->next != NULL && pos->prev != NULL' failed
>
>
> For example (first analysis done by Yonit):
> In red_worker.c (WORKER_FOREACH_DCC):
>    red_pipes_add_drawable
>      red_pipe_add_drawable
>        red_handle_drawable_surfaces_client_synced
>          red_push_surface_image
>            red_channel_client_push
>              red_channel_client_send
>                red_peer_handle_outgoing
>                  reds_stream_writev (if fails -- EPIPE)
>                    handler->cb->on_error = red_channel_client_disconnect()
>                      red_channel_remove_client()
>                        ring_remove() -- of rcc from channel.clients ring.
>
>
> In red_channel.c:
> Commit 53488f0275d6c8a121af49f7ac817d09ce68090d fixes two cases of
> such FOREACH loops.
>
> Since the additional cost of a SAFE loop is insignificant, I replaced
> all such FOREACH macros in red_worker.
>
> In most pacthes, of red_worker, a FOREACH macro is modified to be safe.
> In the last patch of red_worker, a generic SAFE_FOREACH is introduced.
>
> If people prefer, I can reorder the patches such that generic macro is
> introduced in the first patch, and the other patches to use it.
>
> Last, the FOREACH macros can be used outside of red_worker.c.
> Maybe in the future we can move it to an h-file and use it e.g. in red_worker.c
>
>
> Uri Lublin (8):
>    red_worker: use only RCC_FOREACH_SAFE
>    red_worker: reuse DCC_FOREACH in WORKER_DCC_FOREACH
>    red_worker: make WORKER_FOREACH_DCC safe
>    red_worker: use only DRAWABLE_FOREACH_GLZ_SAFE
>    red_worker: make DRAWABLE_FOREACH_DPI safe
>    red_worker: make CCC_FOREACH safe
>    red_worker: use a generic SAFE_FOREACH macro
>    red_channel: replace RING_FOREACH with RING_FOREACH_SAFE in some
>      places
>
>   server/red_channel.c |    8 +-
>   server/red_worker.c  |  190 +++++++++++++++++++++++---------------------------
>   2 files changed, 90 insertions(+), 108 deletions(-)
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>