[Spice-devel,spice-gtk,01/10] spice-channel: Fix a possible race triggered by spice_channel_iterate_write

Submitted by Hans de Goede on Aug. 12, 2011, 9:50 p.m.

Details

Message ID 1313160637-5187-1-git-send-email-hdegoede@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Hans de Goede Aug. 12, 2011, 9:50 p.m.
Fix a race between spice_channel_buffered_write and
spice_channel_iterate_write.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 gtk/spice-channel.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index 8834143..f367b4d 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1750,9 +1750,22 @@  static void spice_channel_iterate_write(SpiceChannel *channel)
 {
     SpiceChannelPrivate *c = channel->priv;
 
-    if (c->xmit_buffer_size) {
-        spice_channel_write(channel, c->xmit_buffer, c->xmit_buffer_size);
+    if (c->xmit_buffer) {
+        /*
+         * Take ownership of the buffer, so that if spice_channel_write calls
+         * g_io_wait and thus yields to the main context, and that then calls
+         * spice_channel_buffered_write it does not mess with the buffer
+         * being written out.
+         */
+        guint8 *buffer = c->xmit_buffer;
+        int size = c->xmit_buffer_size;
+
+        c->xmit_buffer = NULL;
         c->xmit_buffer_size = 0;
+        c->xmit_buffer_capacity = 0;
+
+        spice_channel_write(channel, buffer, size);
+        g_free(buffer);
     }
 }
 

Comments

Hi,

On Fri, Aug 12, 2011 at 04:50:28PM +0200, Hans de Goede wrote:
> -    if (c->xmit_buffer_size) {
> -        spice_channel_write(channel, c->xmit_buffer, c->xmit_buffer_size);
> +    if (c->xmit_buffer) {
> +        /*
> +         * Take ownership of the buffer, so that if spice_channel_write calls
> +         * g_io_wait and thus yields to the main context, and that then calls
> +         * spice_channel_buffered_write it does not mess with the buffer
> +         * being written out.
> +         */
> +        guint8 *buffer = c->xmit_buffer;
> +        int size = c->xmit_buffer_size;
> +
> +        c->xmit_buffer = NULL;
>          c->xmit_buffer_size = 0;
> +        c->xmit_buffer_capacity = 0;
> +
> +        spice_channel_write(channel, buffer, size);
> +        g_free(buffer);

I don't know if you're planning to emit a lot of messages per second with
the USB channel, having something like
if (c->xmit_buffer == NULL)
    c->xmit_buffer = buffer;
    c->xmit_buffer_capacity = ...;
    c->xmit_buffer_size = 0;
} else {
    g_free (buffer)
}

would save some allocations later on.

I'm ACKing either version.

Christophe