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

[Hans de Goede]

Hi,

First of all, since I think I forgot to say so yesterday:
Many thanks for the review!

On 11/16/2011 01:53 PM, Marc-André Lureau wrote:
> Hi
>

<snip>

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

Agreed, and it will. In the unlike scenario that the session releases its
last reference to the channel while it is waiting for the acl_helper,
then it won't get finalized until the helper has completed since priv->result
holds a referenced to self, and priv->result won't get unrefed (which
will cause the self ref to be unreffed once the completion of priv->result
has been done in idle) until the acl_helper has completed. This means that either:
1) There are no active references at dispose time
2) There are active refs but they get immediately unreffed by disconnect
3) There are active and we're waiting for the acl_helper, in the case the
finalize won't happen until the acl_helper_open has completed, at which
point all refs we hold will have been unreffed by the acl open complete
callback.

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

This is not an issue, because of the same reasoning as above.

>
>>>> +#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 :)

I've reworked the usb-channel code to keep track of its state in a state
variable and decide what to do in disconnect based on the state. This makes
the code much clearer. I've also added a comment explaining why just doing
a disconnect on dispose, and not having any finalize at all (see new version
of the patchset) is enough to ensure we always properly clean up after
ourselves.

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

I've discussed this with Kay, but both of us had a rather high reply latency,
so there is no conclusion yet, to be continued.

Regards,

Hans


p.s.

New revision of the patchset should land in your mailbox in a couple of
minutes