[Spice-devel] [PATCH 3/3] channel: make sure we retain RedChannelClient while processing it

[Jonathon Jongsma]

On Wed, 2015-12-09 at 13:36 +0000, Frediano Ziglio wrote:
> During display_channel_handle_migrate_data the pointer is passed
> to different functions which could release it making the pointer
> invalid. Make sure pointer is not freed while processing.

Maybe I didn't go down every path, but I didn't see any cases where the channel
client is freed during processing. But regardless, I'd say it's a good idea to
maintain a reference during processing.

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


> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/display-channel.c | 8 ++++++--
>  server/red-channel.c     | 6 ++----
>  server/red-channel.h     | 2 ++
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 6b1043b..fe05121 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1204,6 +1204,7 @@ int display_channel_handle_migrate_data(DisplayChannel
> *display)
>      uint64_t end_time = red_get_monotonic_time() +
> DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
>      RedChannel *channel = &display->common.base;
>      RedChannelClient *rcc;
> +    int ret = FALSE;
>  
>      if (!red_channel_is_waiting_for_migrate_data(&display->common.base)) {
>          return FALSE;
> @@ -1214,6 +1215,7 @@ int display_channel_handle_migrate_data(DisplayChannel
> *display)
>  
>      rcc = SPICE_CONTAINEROF(ring_get_head(&channel->clients),
> RedChannelClient, channel_link);
>  
> +    red_channel_client_ref(rcc);
>      for (;;) {
>          red_channel_client_receive(rcc);
>          if (!red_channel_client_is_connected(rcc)) {
> @@ -1221,7 +1223,8 @@ int display_channel_handle_migrate_data(DisplayChannel
> *display)
>          }
>  
>          if (!red_channel_client_is_waiting_for_migrate_data(rcc)) {
> -            return TRUE;
> +            ret = TRUE;
> +            break;
>          }
>          if (red_get_monotonic_time() > end_time) {
>              spice_warning("timeout");
> @@ -1230,7 +1233,8 @@ int display_channel_handle_migrate_data(DisplayChannel
> *display)
>          }
>          usleep(DISPLAY_CLIENT_RETRY_INTERVAL);
>      }
> -    return FALSE;
> +    red_channel_client_unref(rcc);
> +    return ret;
>  }
>  
>  void display_channel_flush_all_surfaces(DisplayChannel *display)
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 4b1d426..7dfb9bf 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -116,8 +116,6 @@ static inline int
> red_channel_client_waiting_for_ack(RedChannelClient *rcc);
>  */
>  static void red_channel_ref(RedChannel *channel);
>  static void red_channel_unref(RedChannel *channel);
> -static void red_channel_client_ref(RedChannelClient *rcc);
> -static void red_channel_client_unref(RedChannelClient *rcc);
>  
>  static uint32_t full_header_get_msg_size(SpiceDataHeaderOpaque *header)
>  {
> @@ -1231,12 +1229,12 @@ static void red_channel_unref(RedChannel *channel)
>      }
>  }
>  
> -static void red_channel_client_ref(RedChannelClient *rcc)
> +void red_channel_client_ref(RedChannelClient *rcc)
>  {
>      rcc->refs++;
>  }
>  
> -static void red_channel_client_unref(RedChannelClient *rcc)
> +void red_channel_client_unref(RedChannelClient *rcc)
>  {
>      if (!--rcc->refs) {
>          spice_debug("destroy rcc=%p", rcc);
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 8206cec..2058350 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -418,6 +418,8 @@ int red_channel_is_waiting_for_migrate_data(RedChannel
> *channel);
>  
>  void red_channel_client_destroy(RedChannelClient *rcc);
>  void red_channel_destroy(RedChannel *channel);
> +void red_channel_client_ref(RedChannelClient *rcc);
> +void red_channel_client_unref(RedChannelClient *rcc);
>  
>  int red_channel_client_test_remote_common_cap(RedChannelClient *rcc, uint32_t
> cap);
>  int red_channel_client_test_remote_cap(RedChannelClient *rcc, uint32_t cap);