[Spice-devel] [PATCH spice-gtk 4/5] Add a suid root helper to open usb device nodes

[Marc-André Lureau]

Hi

On Tue, Nov 15, 2011 at 10:20 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> Ah, so you want to just make it a mandatory dep for usb-redir. The problem
> with
> that is that usbredir should work on mac os x too (without any changes, not
> tested),
> and with some more work on windows as well, and neither will have policykit.

right, I forgot about other OS (since we don't support them yet).

>>> +        priv->result = result;
>>> +        priv->acl_helper = spice_usb_acl_helper_new();
>>
>> They should be closed&  disposed in dispose imho.
>
> The close, closes stdin of the helper, at which point it will restore
> the acl to its original state and then exit. Which I would rather see done
> sooner then later.
[]
> So the lifetime of the helper should be either one of the connected time of
> the
> channel, or just long enough for the channel code to open the device. Since
> there
> is no need for / use in keeping the helper around after the channel code has
> opened the device node I've choosen for the latter.

I understand you don't want to delay it until dispose. I was thinking
in term of regular object lifecycle, if it gets dispose (even in a
weird state), it shouldn't leak imho and should restore whatever state
it changed if sensible.

>>
>>> +        spice_usb_acl_helper_open_acl(priv->acl_helper,
>>> +                                      g_usb_device_get_bus(device),
>>> +                                      g_usb_device_get_address(device),
>>> +                                      cancellable,
>>> +
>>>  spice_usbredir_channel_open_acl_cb,
>>> +                                      channel);
>>
>> Since the SpiceUsbAclHelper keep running when it's unref, we should
>> give away a reference to the channel.  And probably close&  dispose on
>> channel dispose. See below
>>
>
> I'm afraid don't understand / completely follow what you're trying to say
> here...

I am trying to say that in term of object lifecycle (sorry, again) we
have a problem, since you can unref the SpiceUsbAclHelper from the
channel, but the helper will keep running and eventually reach the cb
with a channel object that it doesn't own. So it might run into
reference issues. Instead, we should give away a ref (wrong imho) or
have a real weak-reference that get notified when channel object is
disposed so we stop the helper from calling it back.

>>> +#if USE_POLKIT
>>> +    /*
>>> +     * If we're still waiting for the acl helper cancel it and don't
>>> clear
>>> +     * priv->device and context, keeping this channel "busy" until
>>> +     * spice_usbredir_channel_open_acl_cb() has run from the main loop,
>>> +     * avoiding a new connect happening before the old connect has
>>> finished.
>>> +     */
>>
>> If it's called from finalize() I don't see how you could keep the
>> channel "busy".
>
> It cannot be called from finalize() while the acl_helper is still active,
> since the async result holds a reference to its source object, and we don't
> unref the result until the acl_open completes at which point acl_helper will
> be unset.

Can you add this comment or reformulate it, it's easy to get it wrong :)


>>> +    if (priv->acl_helper) {
>>> +        spice_usb_acl_helper_close_acl(priv->acl_helper);
>>> +    } else
>>
>> Do we need a else?
>>
>
> Yes, otherwise we could get:
> channel_open_device 1 -> in progress
> channel_close_device()
>  calls spice_usb_acl_helper_close_acl
>  cancels the pending acl open, will complete open in idle!
> channel_open_device 2
>  sets priv->device to new device
> acl_open called from channel_open_device 1 completes
>  complete callback clears priv->device since there is an err (it was
> cancelled)
>
> But now it is clearing the new device from the 2nd open! Hence the else,
> which results in keeping the channel in the open / busy state until the
> acl_open complete callback runs from idle.

hmm ok

> I've discussed this with Kay in the sense of: "Kay, can't we let udev
> do the acl mangling for us like it does for normal devices too" and
> the answer to that was no. Using an other group for usb devices
> and making the helper sgid to this group won't work since changing
> acl's can only be done by the file owner and root. We could change
> the default owner of the usb files to a special user and have the helper
> be suid this user. I'll discuss this with Kay tomorrow.

That's what I had in mind. I don't think that should prevent us from
merging your patches though.

regards

-- 
Marc-André Lureau