Looks fine to me.
Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
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 */
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel