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

[Victor Toso]

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.

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