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

[Frediano Ziglio]

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

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

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

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);
> >
> 
>