Running sessions "lost" if xrdp-sesman restarts

[ben-cohen]

If xrdp restarts then the new process is not aware of any sessions created by the old process, so you can't re-attach to them and they aren't listed by xrdp-sesadmin. Would it be possible for the new process to "discover" them and populate g_sessions?

Test case:

1. Connect to a new xorgxrdp (Xorg) session.
   xfreerdp /v:localhost /u:<user> /p:<password> &

2. List sessions using xrdp-sesadmin. The session created above will be listed.
   xrdp-sesadmin -u=<user> -p=<password> -c=list

3. Restart xrdp. Note that the existing sessions are not disconnected.
   sudo service xrdp restart

4. List sessions using xrdp-sesadmin.
   xrdp-sesadmin -u=<user> -p=<password> -c=list
   Expected behaviour: the session created above should be listed.
   Observed behaviour: "No sessions." (or, prior to xrdp-sesadmin: fix error when there are no sessions #797, "Error getting session list.")

5. Disconnect the RDP client (without logging out) from the RDP session.

6. Attempt to reconnect to the existing session.
   xfreerdp /v:localhost /u:<user> /p:<password> &

(Workaround: to reconnect to the "orphaned" sessions you can set up xvnc to view the appropriate X display.)

Note that the new xrdp would somehow have to obtain the session_item info from each session it discovers. Also the new xrdp wouldn't be the parent of the sesman for each session so it wouldn't receive SIGCHLD when the session ends (i.e. it would have to poll). I would suggest solving both of these using a shared mmap() object.

[metalefty]

Actually, running session will be lost when xrdp-sesman daemon restarts. Restarting only xrdp daemon won't lost session info.

[ben-cohen]

Thanks, yes! I have corrected the issue title.

[metalefty]

I've corrected the title again :)

[ben-cohen]

Oops, thanks!

To elaborate on my suggestion above, xrdp-sesman could open a file say /var/run/xrdp-sesman.sessions and mmap it into memory. It can store each struct session_item (from the g_sessions linked list) in an array in that region rather than allocated by malloc().

Now if xrdp-sesman restarts, the new process can mmap the file and discover the existing sessions. Using mmap has the advantage that the contents can be shared with other processes, so it can be used to communicate between the main xrdp-sesman instance and its session instances - for example, to update the user's idle time. (A shared pthread mutex can be used to synchronise access.)

To determine if a session instance of xrdp-sesman is still running the main instance can no longer listen for SIGCHLD; instead each session could have a "heartbeat" timestamp that it updates in its mmap struct once a minute. If that timestamp is a few minutes out of date then we can assume that the session has ended. (Of course, if the pid is gone or has the wrong name then we can also assume that the session has ended, but checking shared memory is easier than checking /proc. There are some other ways to detect other processes exiting but they aren't portable.)

[speidy]

Ben, I think that if xrdp-sesman restarts it will halt all sessions running under it anyway

On Wed, Jul 12, 2017 at 11:02 PM Ben Cohen ***@***.***> wrote: Oops, thanks! To elaborate on my suggestion above, xrdp-sesman could open a file say /var/run/xrdp-sesman.sessions and mmap it into memory. It can store each struct session_item (from the g_sessions linked list) in an array in that region rather than allocated by malloc(). Now if xrdp-sesman restarts, the new process can mmap the file and discover the existing sessions. Using mmap has the advantage that the contents can be shared with other processes, so it can be used to communicate between the main xrdp-sesman instance and its session instances - for example, to update the user's idle time. (A shared pthread mutex can be used <https://gist.github.com/ben-cohen/67d41a917ffbf7a5da99fd622feb346b> to synchronise access.) To determine if a session instance of xrdp-sesman is still running the main instance can no longer listen for SIGCHLD; instead each session could have a "heartbeat" timestamp that it updates in its mmap struct once a minute. If that timestamp is a few minutes out of date then we can assume that the session has ended. (Of course, if the pid is gone or has the wrong name then we can also assume that the session has ended, but checking shared memory is easier than checking /proc. There are some other ways <http://bewareofgeek.livejournal.com/2945.html> to detect other processes exiting but they aren't portable.) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#800 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADTH1KBsQQs6kTtEnuWKbO-1loEbShl_ks5sNSZDgaJpZM4OP6DQ> .

-- Idan Freiberg PGP FP: 8108 7EC9 806E 4980 75F2 72B3 8AD3 2D04 337B 1F18

[metalefty]

Overall, the feature is nice and I'm very interested in :)

[ben-cohen]

@speidy - That doesn't happen in practice. If it was intended to do that then this issue is fairly pointless! Is there a reason why it should kill the sessions on exit or is it just that it can't yet re-discover them?

Test case:

1. Connect and log in.
   rdesktop localhost &

2. Stop xrdp.
   sudo service xrdp restart

Observed: the xrdp session is still connected and running. (But the new xrdp-sesadmin doesn't know about it as described above.)

The sessions are not killed because:

0. When the daemon instance starts it sets g_pid to the current process id, and installs signal handlers including sig_sesman_shutdown() for SIGINT and SIGTERM. These are inherited by the session instances when they are forked, and not changed thereafter.

1. service xrdp restart calls /etc/init.d/xrdp which uses start-stop-daemon to send SIGINT to the xrdp-sesadmin daemon instance. It doesn't send anything to the session instances.

2. The daemon instance handles SIGINT using sig_sesman_shutdown(). That calls session_sigkill_all(), which sends SIGTERM to each of the session instances.

3. The session instance handles SIGTERM using sig_sesman_shutdown(). But the process id doesn't match g_pid, which is the daemon instance's process id, so it returns without doing anything (except unhelpfully logging "shutting down sesman 1"), but because the signal was handled it continues running.

[speidy]

Maybe you right and thats not happens when xrdp-sesman is running in deamon mode / background mode. When I run sesman in foreground mode and then send sigterm to it, all the sessions being closed. I'll test later if you scenario actually works as you expect.

On Thu, Jul 13, 2017 at 7:54 PM Ben Cohen ***@***.***> wrote: @speidy <https://github.com/speidy> - That doesn't happen in practice. If it was intended to do that then this issue is fairly pointless! Is there a reason why it should kill the sessions on exit or is it just that it can't yet re-discover them? Test case: 1. Connect and log in. rdesktop localhost & 2. Stop xrdp. sudo service xrdp restart Observed: the xrdp session is still connected and running. (But the new xrdp-sesadmin doesn't know about it as described above.) The sessions are not killed because: 1. When the daemon instance starts it sets g_pid to the current process id, and installs signal handlers including sig_sesman_shutdown() for SIGINT and SIGTERM. These are inherited by the session instances when they are forked, and not changed thereafter. 2. service xrdp restart calls /etc/init.d/xrdp which uses start-stop-daemon to send SIGINT to the xrdp-sesadmin daemon instance. It doesn't send anything to the session instances. 3. The daemon instance handles SIGINT using sig_sesman_shutdown(). That calls session_sigkill_all(), which sends SIGTERM to each of the session instances. 4. The session instance handles SIGTERM using sig_sesman_shutdown(). But the process id doesn't match g_pid, which is the daemon instance's process id, so it returns without doing anything (except unhelpfully logging "shutting down sesman 1"), but because the signal was handled it continues running. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#800 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADTH1Dyc-bFnTaOGRMoIJZW0IvYi566_ks5sNkvLgaJpZM4OP6DQ> .

-- Idan Freiberg PGP FP: 8108 7EC9 806E 4980 75F2 72B3 8AD3 2D04 337B 1F18

[speidy]

@ben-cohen I think your'e right.
Thats actually only happens in foreground mode, where you press Ctrl+C so the signal SIGINT is being sent to the forks (sessions) as well.
I like that feature and idea.

+1

[ben-cohen]

@speidy I've just come to the same conclusion about Ctrl+C sending SIGINT to the whole process group. Thanks, I'll try it then...

[ben-cohen]

I have created PR #819 for this. Let me know if you find any bugs or have any comments.

[arnonerba]

I discovered today that the systemd unit files for xrdp.service and xrdp-sesman.service on Ubuntu 20.04 bind the two services together. There doesn't seem to be a way to restart xrdp without also restarting xrdp-sesman on that platform, short of rewriting the unit files.

Any updates on this issue? I was going to reply to @matt335672 in #819, but wanted to avoid adding more comments to a possibly dead PR.

[matt335672]

Hi @arnonerba

I'll split your question into two bits; firstly, this issue under discussion, and secondly a possible way forward for your particular Ubuntu issue.

As regards the current issue, there's quite a lot to do before we get near this.

The current plan is to revamp and simplify all the inter-process comms used by xrdp. At the moment I'm looking at the link between xrdp and xrdp-sesman. The output from this work will allow us to split sesman into two bits - a session header process for each session, and an overall controller. The headers will maintain virtually all the state for each session connection, and the controller will then be little more than a broker which can easily be restarted. It's a similar model to the one used by ssh. When the controller is restarted, any necessary state info can be passed back up from the session headers to the controller.

I'd initially started at the other end, as I was hoping for a quick fix to #1684 which is relatively important. However, this turned out to be a bad approach. Solving the harder problems first is likely to end up with a better result in the long run.

I'm happy to talk more about this. I'm aware I may not have explained the above very well. We are looking at it, but as with most projects we're having to keep a lot of plates spinning at the same time with fairly limited resources.

As regards the Ubuntu unit files, you don't have to re-write these - you can provide overrides pretty easily straightforwardly which do what you want. See systemd.unit(5) an the details on the edit sub-command in systemctl(1) for more info.

Getting the Ubuntu system files changed, requires talking to the Ubuntu team. As a downstream project, our links for communication with Ubuntu/CentOS, etc are the same as everyone else's - raising a fault on Launchpad (Ubuntu) or Bugzilla (CentOS). Don't expect a quick response to this process!

Is that useful information?

[arnonerba]

Thanks, @matt335672! I appreciate the quick and detailed response. That makes a lot of sense, and it's good to hear that this is still being worked on along with everything else.

I'll think about opening a bug on Launchpad. Do you think it makes sense to treat xrdp.service and xrdp-sesman.service completely separately?

[matt335672]

Personally I think the services should be separate, as xrdp doesn't need sesman in some configurations. However, there's a complication which Ubuntu has brought on themselves for what are actually very good reasons.

The Ubuntu packaging guys have set up the xrdp service to run as a non-privileged user, which is a much more secure way of doing things. However, this means they have to jump through a couple more hoops to get the services started. There's a bit of info on this on the wiki.

From what I found when I looked into this, there's no reason why the services need to be coupled, but I could be wrong.

If you're interested in this, I suggest you try editing the service unit descriptions and see how it goes. If you get something useful, please add the changes you've made to this PR as well as raising them in Launchpad. That will be very useful for the rest of the user community.

Let me know if you need a hand with any of this.

[arnonerba]

Hi Matt,

Thanks for the extra analysis. I'm realizing that I'm not going to have the chance to experiment with the unit files anytime soon. If I can find some time to work on this, I'll be sure to report back here.