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

[Jonathon Jongsma]

On Tue, 2016-11-08 at 10:49 +0100, Christophe Fergeau wrote:
> 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 */



That seems like a better solution to me. It doesn't look like
agent_read_complete() ever kept the 'data' buffer around after the
callback returned, so the behavior change shouldn't cause any
additional problems that I can see. I think I would add a comment to
the udscs_read_callbacK() documentation indicating that the buffer will
be freed after the function is called. It's always nice to be explicit
with ownership assumptions if possible so that users don't to try to
keep the dangling pointers around.

Jonathon