[Spice-devel] [PATCH] remove dandling pointer for RedCharDeviceVDIPort

[Uri Lublin]

On 05/03/2016 01:53 PM, Frediano Ziglio wrote:
>>
>> On 05/02/2016 11:25 AM, Frediano Ziglio wrote:
>>> This was caused by commit 1cec1c5118b65124de6bc6f984f376ff4e297bfb
>>> ("reds: Make VDIPortState a GObject") as the lifespan of RedCharDevice
>>> was changed.
>>>
>>> This could be reproduced with:
>>> - start rhel7 machine
>>> - connect remote viewer (RV)
>>> - RV: login
>>> - connect ssh
>>> - SSH: stop agent
>>> - disconnect RV
>>> - SSH: start agent
>>> - connect to RV
>>>
>>> and caused (using address sanitizer):
>> (snipped)
>>>
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> ---
>>>  server/reds.c | 10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/server/reds.c b/server/reds.c
>>> index efd1429..67c262a 100644
>>> --- a/server/reds.c
>>> +++ b/server/reds.c
>>> @@ -603,12 +603,10 @@ void reds_client_disconnect(RedsState *reds,
>>> RedClient *client)
>>>          reds_mig_remove_wait_disconnect_client(reds, client);
>>>      }
>>>
>>
>> Hi Frediano,
>>
>> Shouldn't the patch fix "agent_attached"  value instead of ignoring it ?
>> Why ignore its value here and not in other places ?
>>
>> Regards,
>>      Uri.
>>
>
> Honestly more I look at the patch and this fix and more I think it's all
> a big bug...
> What I know for sure is that this patch fix a dandling pointer.

That's true.
But I think we should have a better fix that does not ignore the value
of agent_attached, or at least we should verify that other uses of this
variable are correct and this is the only place needs fixing.

>
> Coming to your question. agent_attached. Should be a well defined things,
> there is an agent in the guest attached to spice-server. Now... can we have
> more agents or this "agent_attached" is referring to the agent daemon instead
> of the an agent? All code seems to not to handle multiple agents. I don't
> understand if this is a bug/missing feature or agent here refers to the daemon
> which handle multiple agents.

The "agent" here refers to a program on the guest that opened the serial 
device (port). On Linux guests it's spice-vdagentd.

> Handling the agent_attached. Previous code delete the RedCharDevice before
> setting agent_attached to FALSE. This does not make much sense to me! The
> RedCharDevice represents a device created by Qemu (in the normal usage of
> spice-server) so there is no sense in deleting it when an agent is detached.

I think the Qemu device is always there. Qemu lets spice-server know
when the something (agent) is attached (opened) the device.

> Also looking at the if:
>
>     if (red_channel_test_remote_cap(&reds->main_channel->base,
>                                      SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS)) {
> -        red_char_device_destroy(dev->priv->base);
> -        dev->priv->base = NULL;
> +        dev->priv->agent_attached = FALSE;
>      } else {
> -        red_char_device_reset(dev->priv->base);
> +        red_char_device_reset(RED_CHAR_DEVICE(dev));
>      }
>
> sorry.. this code is really messy. Is supposed to reset the vdp (actually it's
> one of the very few function using "vdp" which is quite confusing). It's using
> some RedChannel state (reds->main_channel) which raise some questions:
> - having one main_channel... how we are supposed to handle multiple clients??

Note that the calling function (reds_agent_remove) has a comment saying:
"TODO: agent is broken with multiple clients."

> - why vdp (which is device which is usually supposed to be always present) have
>   to check a RedChannel (which is supposed to be present only when a client is
>   connected) ?
> - if there isn't the SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS behavior is quite
>   different, why?

Backwards compatibility -- see comment above that code:
" resetting and not destroying the dev as a workaround for a bad
   tokens management in the vdagent protocol"
The code assumes this is not an issue when the client supports
SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS and the char-device can be destroyed.

There is only a single main-channel, but there may be several
clients connected to it (each has its own MainChannelClient).

>
> Looking at http://picpaste.com/channels_devices.png and char-device.c one question
> came into my mind. Who is supposed to handle the RedClient pointer inside
> RedCharDeviceClient? IMHO this is quite bad programming style:
> - the pointer is on a base class but not handled by the base class;
> - there are no checks on the base class that the derived classes handle it;
> - RedCharDevice has a red_char_device_client_add but it's not documented that
>   derived classes MUST call red_char_device_client_remove.
> Looks like other RedCharDevice derived classes handle these stuff in a completely
> different way (for instance this reference is handled by on_disconnect callbacks).

I'm not sure, but maybe the vdagent is the oldest one, and is
the only one that needs to be compatible with old clients.


Thanks,
     Uri.


>
> Frediano
>
>
>>> -    if (reds->agent_dev->priv->agent_attached) {
>>> -        /* note that vdagent might be NULL, if the vdagent was once
>>> -         * up and than was removed */
>>> -        if
>>> (red_char_device_client_exists(RED_CHAR_DEVICE(reds->agent_dev), client))
>>> {
>>> -
>>> red_char_device_client_remove(RED_CHAR_DEVICE(reds->agent_dev),
>>> client);
>>> -        }
>>> +    /* note that client might be NULL, if the vdagent was once
>>> +     * up and than was removed */
>>> +    if (red_char_device_client_exists(RED_CHAR_DEVICE(reds->agent_dev),
>>> client)) {
>>> +        red_char_device_client_remove(RED_CHAR_DEVICE(reds->agent_dev),
>>> client);
>>>      }
>>>
>>>      ring_remove(&client->link);
>>>
>>
>>