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

Wed May 4 08:51:36 UTC 2016

> 
> 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.
> 

Don't be confused by the check removal. Previously (before the offending
commit) the check was checking the existence of RedCharDevice. Now
RedCharDevice is always present so the check should be removed.

I don't see any reason why this patch should not be merged (beside the
comment typo fix).

All other reasoning came from looking the current state of the related
code.

> >
> > 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.
> 

Ok, so mainly spice-vdagentd should deal with multiple guest agents (for
instance multiple users logged in). Make sense.

> > 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.
> 

The fact that the device is removed if there are no agent make me think that
then the object should be related to the agent, not to the device.
If the agent is restarted inside the guest and a client is connected why
deleting all RedCharDeviceClient and RedCharDevice?
I have to look more at the code but looks like this could cause synchronization
problems between the client and spice-server.
>From my current understanding/reasoning a RedCharDeviceClient should still
be present, just in a "disconnected" state. Unless this state cannot exist.

> > 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."
> 

Yes, I would count this as a kind of bug.

Note that previously there was a call to red_char_device_destroy while now
just an assignment, this is a quite big regression in behavior. I think we
should at least call red_char_device_reset.

> > - 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.
> 

Ok, I got more clear now. Basically with SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS
spice-server can tell client that the agent is no more connected so the client
will free its resource and we can close the "connection" (in this case the
RedCharDeviceClient) between the client and the agent and setup again later.
Now this could mean (not tested) that code say to the client the agent is no
more there but still send some data from the old agent.

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

Another kind of bug. I think to support multiple clients this main_channel
pointer has just to disappear.

> >
> > 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.
> 

Maybe. Also true that the agent don't have a specific RedChannel like the
other devices.
As you noted from IRC there are some documentation in char-device.h.

Frediano