[systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt

[David Herrmann]

Hi

On Mon, Aug 11, 2014 at 5:37 PM, Olivier Brunel <jjk at jjacky.com> wrote:
> On 08/11/14 17:12, David Herrmann wrote:
>> Wait, what? Can you please elaborate. Currently, only one process can
>
> Sorry, I meant e.g. having one rootless X on tt1 and starting another
> one on tty2. Currently this fails (see other patch/mail), and is how
> this was observed.

Ah, so it's triggered by the same SIG-handler bug.

>> be controller at a time, and session_prepare_vt() is called *after*
>> the controller is set. Therefore, it is not called when you try
>> starting a second controller.
>>
>> The only race I see is this:
>>  * start legacy Xserver (which doesn't use TakeControl()) which sets
>> user-id on the TTY
>>  * start new Xserver which uses TakeControl() (and thus calls
>> session_prepare_vt())
>>  * stop new Xserver (thus drop Control and reset the TTY)
>>
>> In this case, the new xserver will try to start up, but fail as the
>> old one is still running. Therefore, it *might* call ReleaseControl()
>> and thus reset the TTY. However, you're not supposed to mix both and
>> this is not a legitimate use-case. I mean, the old server is root and
>> modifies the TTY by itself (without using systemd). Obviously, this is
>> racy.
>>
>> From a systemd-logind perspective, this is the same as if you run
>> "sudo chown xyz /dev/tty" manually.
>>
>>> session_prepare_vt() could also check the owner first, and note what to
>>> reset to on session_restore_vt(), to effectively restore things as they
>>> were.
>>
>> Even though I don't see any problem here, I'd be fine with such a
>> patch. Care to send one? Note that you probably need to store that
>> information in the session-file (session_save() / session_load()) too.
>
> If you think the current behavior (setting to root on
> session_restore_vt()) is fine, I'm fine leaving it as is. Else, I could
> work on a patch as described.

I just wanted to understand why that happens. And indeed, by fixing
the SIG-handler bug, this is no longer necessary. I added log_error()
statements to prepare_vt() so the log should be verbose enough. If you
want to fix the fchown() thing, feel free to provide a patch. But I
doubt anyone can trigger it with my fix applied.

Thanks a lot!
David