[Spice-devel] [spice-gtk v2] file-xfer: fix segfault on agent disconnection

[Fabiano Fidêncio]

Toso,

On Sat, May 14, 2016 at 10:09 PM, Victor Toso <lists at victortoso.com> wrote:
> Hi,
>
> On Sat, May 14, 2016 at 05:56:45PM +0200, Fabiano Fidêncio wrote:
>> On May 14, 2016 17:29, "Victor Toso" <victortoso at redhat.com> wrote:
>> >
>> > We are checking self->priv->error but accessing the argument GError *
>> > which is NULL and leads to a segfault.
>> >
>> > Program received signal SIGSEGV, Segmentation fault.
>> > spice_file_transfer_task_completed (self=self at entry=0x7fffd0006f00,
>> error=0x0) at channel-main.c:2963
>> > 2963            VDAgentFileXferStatusMessage msg = {
>> > (gdb) bt
>> >  #0  spice_file_transfer_task_completed (self=self at entry=0x7fffd0006f00,
>> error=0x0) at channel-main.c:2963
>> >  #1  in file_xfer_data_flushed_cb (source_object=0x7cc1d0, res=0x953390,
>> user_data=user_data at entry=0x7fffd0006f00) at channel-main.c:1857
>> >  #2  in g_task_return_now (task=0x953390) at gtask.c:1108
>> >  #3  in g_task_return (task=0x953390, type=<optimized out>) at
>> gtask.c:1166
>> >  #4  in flush_foreach_remove (key=<optimized out>, value=<optimized out>,
>> user_data=<optimized out>) at channel-main.c:928
>> >  #5  in g_hash_table_foreach_remove_or_steal (hash_table=0x70cea0,
>> func=func at entry=0x7ffff5616f10 <flush_foreach_remove>,
>> user_data=user_data at entry=0x0, notify=notify at entry=1) at ghash.c:1492
>> >  #6  in g_hash_table_foreach_remove (hash_table=<optimized out>,
>> func=func at entry=0x7ffff5616f10 <flush_foreach_remove>,
>> user_data=user_data at entry=0x0) at ghash.c:1538
>> >  #7  in file_xfer_flushed (success=0, channel=0x7cc1d0) at
>> channel-main.c:936
>> >  #8  spice_main_channel_reset_agent (channel=0x7cc1d0) at
>> channel-main.c:466
>> >  #9  set_agent_connected (channel=0x7cc1d0, connected=connected at entry=0)
>> at channel-main.c:1572
>> >  #10 in spice_main_channel_reset (channel=0x7cc1d0, migrating=0) at
>> channel-main.c:485
>> >  #11 in spice_channel_coroutine (data=0x7cc1d0) at spice-channel.c:2564
>> >  #12 in coroutine_trampoline (cc=0x7cb860) at coroutine_ucontext.c:63
>> >  #13 in continuation_trampoline (i0=<optimized out>, i1=<optimized out>)
>> at continuation.c:55
>> >  #14 in ?? () from /lib64/libc.so.6
>> >  #15 in ?? ()
>> >  #16 in ?? ()
>> > Backtrace stopped: Cannot access memory at address
>> > ---
>> >  src/channel-main.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/channel-main.c b/src/channel-main.c
>> > index 2905d7b..692cfe7 100644
>> > --- a/src/channel-main.c
>> > +++ b/src/channel-main.c
>> > @@ -2964,7 +2964,7 @@ static void
>> spice_file_transfer_task_completed(SpiceFileTransferTask *self,
>> >      if (self->priv->error) {
>> >          VDAgentFileXferStatusMessage msg = {
>> >              .id = self->priv->id,
>> > -            .result = error->code == G_IO_ERROR_CANCELLED ?
>> > +            .result = self->priv->error->code == G_IO_ERROR_CANCELLED ?
>> >                      VD_AGENT_FILE_XFER_STATUS_CANCELLED :
>> VD_AGENT_FILE_XFER_STATUS_ERROR,
>> >          };
>> >          agent_msg_queue_many(self->priv->channel,
>> VD_AGENT_FILE_XFER_STATUS,
>> > --
>> > 2.5.5
>> >
>>
>> Victor, as we talked on IRC your patch solves the problem, fust to comments:
>>
>> First one, shouldn't we emmit self->priv->error instead of error in the end
>> of this function?
>>
>> Second one, from a quick look in the code, I have the impression that we
>> can get rid of the private Error variable and just use the error that we
>> are propagating between the callbacks. A deeper read in the code would be
>> needed though in order to affirm that.
>>
>> Best Regards,
>
> I would rather that we fix the segfault in one patch and improve the
> logic in a different patch/series. I plan to look into this code
> carefully during this next week but I hope your suggestions are not a
> blocker.

The suggestion about emitting the signal, if needed, is a blocker.
The other one may be done while you re-work this code path.

>
>   toso


Best Regards,
-- 
Fabiano Fidêncio