[Spice-devel] [PATCH v2] channel: do not free rcc->stream in red_channel_client_disconnect

[Jonathon Jongsma]

On Tue, 2016-01-19 at 10:36 +0000, Frediano Ziglio wrote:
> This fixes a crash if red_channel_client disconnect is called
> handling a message.

"called while handling"

> This can happen for instance handling SPICE_MSGC_ACK which calls

"while handling"

> red_channel_client_push which try to write items to socket detecting
> writing errors (for instance socket disconnection).

This sentence is a bit unclear to me. How about "which tries to detect write
errors while writing to a socket"?


> Messages are read in a loop and the red_channel_client_disconnect

s/the//

> would cause rcc->stream to be NULL which will turn in a use-after-free

s/turn in/result in/

> problem (stream in red_peer_handle_incoming will use cached stream value).


ACK with above changes

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-channel.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Changes from v1:
> - reduce diff size removing style change.
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 306c87d..efe889a 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1233,6 +1233,10 @@ void red_channel_client_unref(RedChannelClient *rcc)
>  {
>      if (!--rcc->refs) {
>          spice_debug("destroy rcc=%p", rcc);
> +
> +        reds_stream_free(rcc->stream);
> +        rcc->stream = NULL;
> +
>          if (rcc->send_data.main.marshaller) {
>              spice_marshaller_destroy(rcc->send_data.main.marshaller);
>          }
> @@ -1749,7 +1753,7 @@ void red_channel_pipes_add_empty_msg(RedChannel
> *channel, int msg_type)
>  int red_channel_client_is_connected(RedChannelClient *rcc)
>  {
>      if (!rcc->dummy) {
> -        return rcc->stream != NULL;
> +        return ring_item_is_linked(&rcc->channel_link);
>      } else {
>          return rcc->dummy_connected;
>      }
> @@ -1804,6 +1808,8 @@ static void red_channel_remove_client(RedChannelClient
> *rcc)
>                        rcc->channel->type, rcc->channel->id,
>                        rcc->channel->thread_id, pthread_self());
>      }
> +    spice_return_if_fail(ring_item_is_linked(&rcc->channel_link));
> +
>      ring_remove(&rcc->channel_link);
>      spice_assert(rcc->channel->clients_num > 0);
>      rcc->channel->clients_num--;
> @@ -1845,8 +1851,6 @@ void red_channel_client_disconnect(RedChannelClient
> *rcc)
>          rcc->channel->core->watch_remove(rcc->stream->watch);
>          rcc->stream->watch = NULL;
>      }
> -    reds_stream_free(rcc->stream);
> -    rcc->stream = NULL;
>      if (rcc->latency_monitor.timer) {
>          rcc->channel->core->timer_remove(rcc->latency_monitor.timer);
>          rcc->latency_monitor.timer = NULL;