[Spice-devel] udscs: The read buffer is always reset to NULL

Submitted by Francois Gouget on Nov. 28, 2016, 8:40 a.m.

Details

Message ID E1cBHUe-0008KT-MZ@amboise
State Accepted
Commit c943d0b6b2b25aa0e69857ae5e1f987d0010a169
Headers show
Series "udscs: Fix memory ownership issues with udscs_read_callback" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Nov. 28, 2016, 8:40 a.m.
Tweak the code to clarify that conn->data.buf is set to NULL after the
free() in udscs_read_complete().
Note that this reset is needed to avoid a double-free in
udscs_destroy_connection() if an error occurs while receiving
the next message header.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---

On Fri, 25 Nov 2016, Christophe Fergeau wrote:
>
> This memset should 'buf' to NULL. (can't remember if I took that into
> account while writing the patch though :)

Maybe tweaking the code like below could help make this clearer. The 
comment may be overkill, or maybe on the contrary it should be made more 
explicit.


 src/udscs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/udscs.c b/src/udscs.c
index b468e71..414dce5 100644
--- a/src/udscs.c
+++ b/src/udscs.c
@@ -236,10 +236,10 @@  static void udscs_read_complete(struct udscs_connection **connp)
         if (!*connp) /* Was the connection disconnected by the callback ? */
             return;
     }
-    free(conn->data.buf);
 
+    free(conn->data.buf);
+    memset(&conn->data, 0, sizeof(conn->data)); /* data.buf = NULL */
     conn->header_read = 0;
-    memset(&conn->data, 0, sizeof(conn->data));
 }
 
 /* A helper for udscs_client_handle_fds() */

Comments

On Mon, Nov 28, 2016 at 09:40:56AM +0100, Francois Gouget wrote:
> Tweak the code to clarify that conn->data.buf is set to NULL after the
> free() in udscs_read_complete().
> Note that this reset is needed to avoid a double-free in
> udscs_destroy_connection() if an error occurs while receiving
> the next message header.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> On Fri, 25 Nov 2016, Christophe Fergeau wrote:
> >
> > This memset should 'buf' to NULL. (can't remember if I took that into
> > account while writing the patch though :)
> 
> Maybe tweaking the code like below could help make this clearer. The 
> comment may be overkill, or maybe on the contrary it should be made more 
> explicit.

Does not hurt to make this explicit indeed,

Acked-by: Christophe Fergeau <cfergeau@redhat.com>

Christophe