[Spice-devel] [PATCH] usbredir: fix redirection of user-accesible device nodes.

[Christophe Fergeau]

On Thu, Jul 09, 2015 at 02:35:58PM +0200, Michal Suchanek wrote:
> Excerpts from Christophe Fergeau's message of Thu Jul 09 13:47:51 +0200 2015:
> > On Mon, Jun 29, 2015 at 03:46:56PM +0200, Michal Suchanek wrote:
> > > When calling ACL helper fails also try to open the device node directly.
> > > 
> > > Otherwise user-accessible device nodes are rejected when policykit
> > > support is compiled in and policy is not set up when in fact the device
> > > could be accessed.
> > > 
> > > Signed-off-by: Michal Suchanek <hramrach at gmail.com>
> > > ---
> > >  src/channel-usbredir.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > > index 292b82f..b5745b6 100644
> > > --- a/src/channel-usbredir.c
> > > +++ b/src/channel-usbredir.c
> > > @@ -276,21 +276,24 @@ static void spice_usbredir_channel_open_acl_cb(
> > >      SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(user_data);
> > >      SpiceUsbredirChannelPrivate *priv = channel->priv;
> > >      GError *err = NULL;
> > > +    GError *acl_err = NULL;
> > >  
> > >      g_return_if_fail(acl_helper == priv->acl_helper);
> > >      g_return_if_fail(priv->state == STATE_WAITING_FOR_ACL_HELPER ||
> > >                       priv->state == STATE_DISCONNECTING);
> > >  
> > > -    spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &err);
> > > -    if (!err && priv->state == STATE_DISCONNECTING) {
> > > +    spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &acl_err);
> > 
> > I assume in your setup, 'acl_err' gets set when calling open_acl_finish?
> > What is the error message you are getting? (ideally error domain/code
> > would be nice too).
> 
> I have no policykit set up so every attempt to open anything is denied
> due to insufficient permissions. I can get the exact error if it's
> important.
> 
> > 
> > > +    if (!acl_err && priv->state == STATE_DISCONNECTING) {
> > >          err = g_error_new_literal(G_IO_ERROR, G_IO_ERROR_CANCELLED,
> > >                                    "USB redirection channel connect cancelled");
> > >      }
> > > -    if (!err) {
> > > -        spice_usbredir_channel_open_device(channel, &err);
> > > -    }
> > > +
> > > +    spice_usbredir_channel_open_device(channel, &err);
> > 
> > given that you added separate acl_err and err, you can keep the if
> > (!err) { } unchanged here I think
> 
> err is NULL here so it's nop either way.

Right, sorry ;)

> 
> > 
> > >      if (err) {
> > 
> > but here this will need to be if (err || acl_err) {
> 
> No. If err is NULL then open succeeded. Err is set by actually opening
> the device.


Ah correct too, acl_err is only used in order to be able to propagate
the right error.

> 
> > 
> > > -        g_simple_async_result_take_error(priv->result, err);
> > > +        if (acl_err)
> > > +            g_simple_async_result_take_error(priv->result, acl_err);
> > > +        else
> > > +            g_simple_async_result_take_error(priv->result, err);
> > 
> > If open_device() failed after open_acl_finish() failing, both acl_err
> > and err will be set, so you need to free the one you are not going to
> > use.
> 
> Which is the case when calling out to the policykit does not give
> permission to open and the open actually fails.
> 
> > I'd do 
> > if (acl_err) {
> >     g_simple_async_result_take_error(priv->result, acl_err);
> >     acl_err = NULL;
> > } else {
> >     g_simple_async_result_take_error(priv->result, err);
> >     err = NULL;
> > }
> > g_clear_error(acl_err);
> > g_clear_error(err);
> 
> If that's the way to handle the errors it's easy to add.

This is one way of making sure no memory is leaked. The g_clear_error()
will need to be outside the if (err) {} block as acl_err can be set when
err is not set.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150709/86440670/attachment.sig>