[spice-gtk,1/2] vmcstream: fix task cancellation

Submitted by Jakub Janku on Aug. 20, 2019, 2:05 p.m.

Details

Message ID 20190820140547.15251-1-jjanku@redhat.com
State Accepted
Commit 69e90462855ac74de2ffae5484efc48a4b99d0ff
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jakub Janku Aug. 20, 2019, 2:05 p.m.
If the task is cancelled, g_task_propagate_pointer()
in spice_vmc_output_stream_write_finish() returns NULL
which produces following errors:

    (spicy:13597): GSpice-CRITICAL **: 15:39:46.168: spice_vmc_write_finish: assertion 'result != NULL' failed

    (spicy:13597): GLib-GObject-CRITICAL **: 15:39:46.168: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

To fix this, call spice_vmc_write_finish() immediately in the
corresponding write_cb().

Signed-off-by: Jakub Janků <jjanku@redhat.com>
---
 src/vmcstream.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vmcstream.c b/src/vmcstream.c
index b6f6d1a..b7da103 100644
--- a/src/vmcstream.c
+++ b/src/vmcstream.c
@@ -397,18 +397,11 @@  spice_vmc_output_stream_write_fn(GOutputStream   *stream,
 
 static gssize
 spice_vmc_output_stream_write_finish(GOutputStream *stream,
-                                     GAsyncResult *simple,
+                                     GAsyncResult *result,
                                      GError **error)
 {
-    SpiceVmcOutputStream *self = SPICE_VMC_OUTPUT_STREAM(stream);
-    GAsyncResult *res = g_task_propagate_pointer(G_TASK(simple), error);
-    gssize bytes_written;
-
-    SPICE_DEBUG("spicevmc write finish");
-    bytes_written = spice_vmc_write_finish(self->channel, res, error);
-    g_object_unref(res);
-
-    return bytes_written;
+    g_return_val_if_fail(g_task_is_valid(result, stream), -1);
+    return g_task_propagate_int(G_TASK(result), error);
 }
 
 static void
@@ -417,9 +410,17 @@  write_cb(GObject *source_object,
          gpointer user_data)
 {
     GTask *task = user_data;
+    GError *error = NULL;
+    gssize bytes_written;
 
-    g_task_return_pointer(task, g_object_ref(res), g_object_unref);
+    SPICE_DEBUG("spicevmc write finish");
+    bytes_written = spice_vmc_write_finish(SPICE_CHANNEL(source_object), res, &error);
 
+    if (error) {
+        g_task_return_error(task, error);
+    } else {
+        g_task_return_int(task, bytes_written);
+    }
     g_object_unref(task);
 }
 

Comments


> 
> Hi,
> 
> On Tue, Aug 20, 2019 at 04:05:46PM +0200, Jakub Janků wrote:
> > If the task is cancelled, g_task_propagate_pointer()
> > in spice_vmc_output_stream_write_finish() returns NULL
> > which produces following errors:
> > 
> >     (spicy:13597): GSpice-CRITICAL **: 15:39:46.168:
> >     spice_vmc_write_finish: assertion 'result != NULL' failed
> > 
> >     (spicy:13597): GLib-GObject-CRITICAL **: 15:39:46.168: g_object_unref:
> >     assertion 'G_IS_OBJECT (object)' failed
> 
> What I found weird was actually that we do _ref() on write_cb()
> but this got cleanup anyway, seems like a bug somewhere.
> 

It's not a bug, as Jakub said and from g_task_propagate_pointer
documentation "If the task resulted in an error, or was cancelled,
then this will instead return NULL and set error."

So spice_vmc_output_stream_write_finish called g_task_propagate_pointer
and passed it as "result" to spice_vmc_write_finish which trigger
the "assertion 'result != NULL'" error. When spice_vmc_write_finish
returned a g_object_unref is called on the NULL object causing
the "assertion 'G_IS_OBJECT (object)' failed".

> I don't see nothing wrong with your proposal, actually thinks it
> is better to do the write on write_cb() and let the _finish()
> just provide the overall feedback of the _async() call.
> 
> > To fix this, call spice_vmc_write_finish() immediately in the
> > corresponding write_cb().
> > 
> > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> 
> Acked-by: Victor Toso <victortoso@redhat.com>
> 
> > ---
> >  src/vmcstream.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/vmcstream.c b/src/vmcstream.c
> > index b6f6d1a..b7da103 100644
> > --- a/src/vmcstream.c
> > +++ b/src/vmcstream.c
> > @@ -397,18 +397,11 @@ spice_vmc_output_stream_write_fn(GOutputStream
> > *stream,
> >  
> >  static gssize
> >  spice_vmc_output_stream_write_finish(GOutputStream *stream,
> > -                                     GAsyncResult *simple,
> > +                                     GAsyncResult *result,
> >                                       GError **error)
> >  {
> > -    SpiceVmcOutputStream *self = SPICE_VMC_OUTPUT_STREAM(stream);
> > -    GAsyncResult *res = g_task_propagate_pointer(G_TASK(simple), error);
> > -    gssize bytes_written;
> > -
> > -    SPICE_DEBUG("spicevmc write finish");
> > -    bytes_written = spice_vmc_write_finish(self->channel, res, error);
> > -    g_object_unref(res);
> > -
> > -    return bytes_written;
> > +    g_return_val_if_fail(g_task_is_valid(result, stream), -1);
> > +    return g_task_propagate_int(G_TASK(result), error);
> >  }
> >  
> >  static void
> > @@ -417,9 +410,17 @@ write_cb(GObject *source_object,
> >           gpointer user_data)
> >  {
> >      GTask *task = user_data;
> > +    GError *error = NULL;
> > +    gssize bytes_written;
> >  
> > -    g_task_return_pointer(task, g_object_ref(res), g_object_unref);
> > +    SPICE_DEBUG("spicevmc write finish");
> > +    bytes_written = spice_vmc_write_finish(SPICE_CHANNEL(source_object),
> > res, &error);
> >  
> > +    if (error) {
> > +        g_task_return_error(task, error);
> > +    } else {
> > +        g_task_return_int(task, bytes_written);
> > +    }
> >      g_object_unref(task);
> >  }
> >