[Spice-devel] [vd_agent] udscs: Clarify the udscs_read_callback() documentation

[Christophe Fergeau]

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 at codeweavers.com>
> > ---
> >  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 */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161108/7e30edd0/attachment.sig>