[Spice-devel] [spice-gtk v1 06/10] file-transfer: Fix SpiceFileTransferTask::error leak

[Christophe Fergeau]

On Mon, Aug 01, 2016 at 05:24:28PM +0200, Victor Toso wrote:
> Hi,
> 
> On Mon, Aug 01, 2016 at 12:43:10PM +0200, Christophe Fergeau wrote:
> > On Sat, Jul 30, 2016 at 12:26:27AM +0200, Victor Toso wrote:
> > > The self->error is the error set for the file-transfer and it will be
> > > propagated with the "finish" signal. As this is transfer none pointer,
> > > we should clear it out afterwards.
> > >
> > > 40 (16 direct, 24 indirect) bytes in 1 blocks are definitely lost in
> > > loss record 7,489 of 14,298
> > >  at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> > >  by 0xB5090E8: g_malloc (gmem.c:94)
> > >  by 0xB51F8A2: g_slice_alloc (gslice.c:1025)
> > >  by 0xB4EFCC5: g_error_new_literal (gerror.c:471)
> > >  by 0xB4EFFAD: g_set_error_literal (gerror.c:619)
> > >  by 0xAF13397: g_cancellable_set_error_if_cancelled (gcancellable.c:314)
> > >  by 0xAF630C8: g_task_propagate_error (gtask.c:1519)
> > >  by 0xAF63CD8: g_task_propagate_int (gtask.c:1652)
> > >  by 0x50863F5: spice_file_transfer_task_read_finish (spice-file-transfer-task.c:477)
> > >  by 0x5093239: file_xfer_read_async_cb (channel-main.c:1811)
> > >  by 0xAF62F38: g_task_return_now (gtask.c:1121)
> > >  by 0xAF63775: g_task_return (gtask.c:1179)
> > > ---
> > >  src/spice-file-transfer-task.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> > > index ea46c9d..667d230 100644
> > > --- a/src/spice-file-transfer-task.c
> > > +++ b/src/spice-file-transfer-task.c
> > > @@ -589,6 +589,7 @@ spice_file_transfer_task_dispose(GObject *object)
> > >  
> > >      g_clear_object(&self->file);
> > >      g_clear_object(&self->file_stream);
> > > +    g_clear_error(&self->error);
> > >  
> > >      G_OBJECT_CLASS(spice_file_transfer_task_parent_class)->dispose(object);
> > 
> > I think you'll need at least something like
> > diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> > index 70571f0..ba99e1f 100644
> > --- a/src/spice-file-transfer-task.c
> > +++ b/src/spice-file-transfer-task.c
> > @@ -122,6 +122,7 @@ static void spice_file_transfer_task_query_info_cb(GObject *obj,
> >          g_clear_error(&error);
> >          /* Return error previously reported */
> >          g_task_return_error(task, self->error);
> > +        self->error = NULL;
> 
> Not really.
> 

Ah, I was not suggesting changing the overall design, but
g_task_return_error() takes ownership of self->error
« Note that since the task takes ownership of error , and since the task
may be completed before returning from g_task_return_error(), you cannot
assume that error is still valid after calling this. Call g_error_copy()
on the error if you need to keep a local copy ».
I would not call g_clear_error(&self->error); after calling
g_task_return_error() as it's now owned by the GTask, and then by the
caller of g_task_propagate_*

Christophe

> SpiceFileTransferTask::error is NULL when file-transfer start and if it
> is set, it means that file-transfer has failed. We should keep it set
> till the very end unless the error is something we can recover from.
> 
> The g_task_return_error() will inform the caller (SpiceMainChannel) that
> this xfer-task has failed and in that situation it will call
> spice_file_transfer_task_completed(). The GError argument in
> _task_completed() is for external errors (e.g. Agent had an error) so,
> SpiceMainChannel might not call _task_completed() with the GError set.
> 
> If you agree with what I've said, we might improve the documentation to
> have this more clear; changing the design/usage of self->error might be
> possible too, but I don't think it is necessary.
> 
> Thanks for the reviews,
>   toso
> 
> 
> >          g_object_unref(task);
> >          return;
> >      } else if (error) {
> > @@ -160,6 +161,7 @@ static void spice_file_transfer_task_read_file_cb(GObject *obj,
> >          /* Return error previously reported */
> >          self->pending = FALSE;
> >          g_task_return_error(task, self->error);
> > +        self->error = NULL;
> >          g_object_unref(task);
> >          return;
> >      } else if (error) {
> > @@ -198,6 +200,7 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
> >          g_clear_error(&error);
> >          /* On any pending error on SpiceFileTransferTask */
> >          g_task_return_error(task, self->error);
> > +        self->error = NULL;
> >          g_object_unref(task);
> >          return;
> >      } else if (error) {
> > 
> > 
> > (or to g_error_copy() the error you pass to g_task_return_error()).
> > Though I must admit I haven't been able to easily follow the codepaths
> > handling self->error, so maybe I missed something.
> >
> > Christophe
> 
> 
> 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160801/1a61374b/attachment.sig>