[Spice-devel] [PATCH spice-server v2] red-channel-client: Avoid weird memory references using MarkerPipeItem

[Christophe Fergeau]

On Tue, Sep 12, 2017 at 02:14:56PM +0100, Frediano Ziglio wrote:
> Instead of having MarkerPipeItem pointing to an external variable with
> the possibility to forget to reset it and have a dangling pointer use
> reference counting to keep the item and mark the item when sent.

"and have a dangling pointer, this commit takes a reference on the item
to keep it alive after it was sent.


> This item is placed into the queue to understand when it was sent. The
> current implementation detect the unqueue when the item is destroyed so

"detects"

> we use an external variable to use it after the item is released.

maybe "We currently store a pointer to an external variable in the item, this
way we can use a variable which will still be alive after the item is
released/destroyed" ?

> Instead this change update the variable (stored in the item) when the
> item is attempted to be sent. This avoids having to destroy the item.

"This change updates the variable (..) when we try to send the item, rather
than at destruction time."
I would add a mention that before that commit, the destruction happened
at the end of red_channel_client_send_item() (did not check there is no
other ref), so we don't signal "item sent" much earlier than before.

Apart from the log,

Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Christophe

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-channel-client.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 34202c492..de3ac27cb 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -218,7 +218,7 @@ typedef struct RedEmptyMsgPipeItem {
>  
>  typedef struct MarkerPipeItem {
>      RedPipeItem base;
> -    gboolean *item_in_pipe;
> +    bool item_in_pipe;
>  } MarkerPipeItem;
>  
>  static void red_channel_client_start_ping_timer(RedChannelClient *rcc, uint32_t timeout)
> @@ -613,6 +613,7 @@ static void red_channel_client_send_item(RedChannelClient *rcc, RedPipeItem *ite
>              red_channel_client_send_ping(rcc);
>              break;
>          case RED_PIPE_ITEM_TYPE_MARKER:
> +            SPICE_UPCAST(MarkerPipeItem, item)->item_in_pipe = false;
>              break;
>          default:
>              red_channel_send_item(rcc->priv->channel, rcc, item);
> @@ -1757,23 +1758,13 @@ void red_channel_client_set_header_sub_list(RedChannelClient *rcc, uint32_t sub_
>      rcc->priv->send_data.header.set_msg_sub_list(&rcc->priv->send_data.header, sub_list);
>  }
>  
> -static void marker_pipe_item_free(RedPipeItem *base)
> -{
> -    MarkerPipeItem *item = SPICE_UPCAST(MarkerPipeItem, base);
> -
> -    if (item->item_in_pipe) {
> -        *item->item_in_pipe = FALSE;
> -    }
> -    free(item);
> -}
> -
>  /* TODO: more evil sync stuff. anything with the word wait in it's name. */
>  bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>                                              GList *item_pos,
>                                              int64_t timeout)
>  {
>      uint64_t end_time;
> -    gboolean item_in_pipe;
> +    bool item_in_pipe;
>  
>      spice_debug("trace");
>  
> @@ -1785,10 +1776,9 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  
>      MarkerPipeItem *mark_item = spice_new0(MarkerPipeItem, 1);
>  
> -    red_pipe_item_init_full(&mark_item->base, RED_PIPE_ITEM_TYPE_MARKER,
> -                            marker_pipe_item_free);
> -    item_in_pipe = TRUE;
> -    mark_item->item_in_pipe = &item_in_pipe;
> +    red_pipe_item_init(&mark_item->base, RED_PIPE_ITEM_TYPE_MARKER);
> +    mark_item->item_in_pipe = true;
> +    red_pipe_item_ref(&mark_item->base);
>      red_channel_client_pipe_add_after_pos(rcc, &mark_item->base, item_pos);
>  
>      if (red_channel_client_is_blocked(rcc)) {
> @@ -1797,7 +1787,7 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>      }
>      red_channel_client_push(rcc);
>  
> -    while (item_in_pipe &&
> +    while (mark_item->item_in_pipe &&
>             (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
>          usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
>          red_channel_client_receive(rcc);
> @@ -1805,9 +1795,11 @@ bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>          red_channel_client_push(rcc);
>      }
>  
> +    item_in_pipe = mark_item->item_in_pipe;
> +    red_pipe_item_unref(&mark_item->base);
> +
>      if (item_in_pipe) {
> -        // still on the queue, make sure won't overwrite the stack variable
> -        mark_item->item_in_pipe = NULL;
> +        // still on the queue
>          spice_warning("timeout");
>          return FALSE;
>      } else {
> -- 
> 2.13.5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171129/b7c883d3/attachment.sig>