On Wed, Nov 02, 2016 at 04:27:19PM -0500, Jonathon Jongsma wrote:
> On Wed, 2016-11-02 at 16:36 +0100, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> > ---
> > src/udscs.h | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/udscs.h b/src/udscs.h
> > index e13750c..d65630d 100644
> > --- a/src/udscs.h
> > +++ b/src/udscs.h
> > @@ -39,8 +39,9 @@ struct udscs_message_header {
> > };
> >
> > /* Callbacks with this type will be called when a complete message
> > has been
> > - * received. The callback may call udscs_destroy_connection, in
> > which case
> > - * *connp must be made NULL (which udscs_destroy_connection takes
> > care of).
> > + * 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).
> > */
> > typedef void (*udscs_read_callback)(struct udscs_connection **connp,
> > struct udscs_message_header *header, uint8_t *data);
>
>
> From reading the code, it looks like the data buffer should *NOT* be
> freed if udscs_destroy_connection() is called, however, since this
> buffer is owned by the connection and will be freed when the connection
> is destroyed. It might be worth pointing out that potential pitfall.
It's not only potential as we seem to have a few double free. I'd go
with a patch like the one below (which makes that comment patch obsolete
though)
>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/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 */
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel