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

[Jonathon Jongsma]

Looks fine to me.

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


On Wed, 2016-11-09 at 11:47 +0100, Christophe Fergeau wrote:
> Before this commit, udscs_read_callback is supposed to free the
> 'data'
> buffer unless it calls udscs_destroy_connection(). It's currently not
> doing this, causing a potential double free in error cases. The udscs
> code would also leak the 'data' buffer if no udscs_read_callback is
> set.
> 
> This commit moves ownership of the 'data' buffer to the generic code
> calling the udscs_read_callback and fixes the memory management of
> this
> 'data' buffer. As the only udscs_read_callback currently implemented
> is
> not keeping pointers to 'data' after it returns, this should not
> cause
> any issues.
> ---
>  src/udscs.c             | 2 ++
>  src/udscs.h             | 7 ++++---
>  src/vdagentd/vdagentd.c | 4 ----
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> 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);
>  
>      conn->header_read = 0;
>      memset(&conn->data, 0, sizeof(conn->data));
> diff --git a/src/udscs.h b/src/udscs.h
> index d65630d..6e960f8 100644
> --- a/src/udscs.h
> +++ b/src/udscs.h
> @@ -39,9 +39,10 @@ struct udscs_message_header {
>  };
>  
>  /* Callbacks with this type will be called when a complete message
> has been
> - * received. The callback is responsible for free()-ing the data
> buffer. It
> - * may call udscs_destroy_connection, in which case *connp must be
> made NULL
> - * (which udscs_destroy_connection takes care of).
> + * received. The callback does not own the data buffer and should
> not free it.
> + * The data buffer will be freed shortly after the read callback
> returns.
> + * The callback may call udscs_destroy_connection, in which case
> *connp must be
> + * made NULL (which udscs_destroy_connection takes care of).
>   */
>  typedef void (*udscs_read_callback)(struct udscs_connection **connp,
>      struct udscs_message_header *header, uint8_t *data);
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 18e82a7..a1faf23 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -727,7 +727,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>          if (header->arg1 == 0 && header->arg2 == 0) {
>              syslog(LOG_INFO, "got old session agent xorg resolution
> message, "
>                               "ignoring");
> -            free(data);
>              return;
>          }
>  
> @@ -735,7 +734,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>              syslog(LOG_ERR, "guest xorg resolution message has wrong
> size, "
>                              "disconnecting agent");
>              udscs_destroy_connection(connp);
> -            free(data);
>              return;
>          }
>  
> @@ -760,7 +758,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>      case VDAGENTD_CLIPBOARD_RELEASE:
>          if (do_agent_clipboard(*connp, header, data)) {
>              udscs_destroy_connection(connp);
> -            free(data);
>              return;
>          }
>          break;
> @@ -783,7 +780,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>          syslog(LOG_ERR, "unknown message from vdagent: %u,
> ignoring",
>                 header->type);
>      }
> -    free(data);
>  }
>  
>  /* main */