[Spice-devel] [vdagent-linux] udscs: Fix memory ownership issues with udscs_read_callback

[Francois Gouget]

Thanks for this patch, I think it makes the code more consistent and 
cleaner. While rechecking this I think I found a wrinkle though:


On Wed, 9 Nov 2016, Christophe Fergeau wrote:
[...]
> diff --git a/src/udscs.c b/src/udscs.c
> index 427a844..b468e71 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -132,6 +132,7 @@ void udscs_destroy_connection(struct udscs_connection **connp)
>      }
>  
>      free(conn->data.buf);
> +    conn->data.buf = NULL;
>  
>      if (conn->next)
>          conn->next->prev = conn->prev;
> @@ -235,6 +236,7 @@ static void udscs_read_complete(struct udscs_connection **connp)
>          if (!*connp) /* Was the connection disconnected by the callback ? */
>              return;
>      }
> +    free(conn->data.buf);
>

Why is conn->data.buf not set to NULL here?

I believe it can lead to a double-free:
 * Assume we received a message so that udscs_read_complete() was called 
   and freed conn->data.buf.
 
 * Then we receive another message but an error occurs in 
   udscs_do_read() while receiving the header. This leads to

    n = read(conn->fd, dest, to_read);
    ...
    if (n <= 0) {
        udscs_destroy_connection(connp);

   At that point conn->data.buf is still non-NULL so 
   udscs_destroy_connection() will free it again.


-- 
Francois Gouget <fgouget at codeweavers.com>