Race condition in suspend scripts reveals desktop

Status changed to 'Confirmed' because the bug affects multiple users.

I can confirm this on xscreensaver 5.21 on Debian unstable with xfce as well. The problem is that the xscreensaver daemon is told to lock the screen only after the computer has woken up. Using xscreensaver -debug I can see that it receives the THROTTLE ClientMessage before the sleep, but the LOCK ClientMessage after the sleep.

/etc/acpi/sleep_suspend.sh calls /usr/share/acpi-support/screenblank (which runs "xscreensaver-command -throttle" then "xscreensaver-command -lock" in the background) just before running pm-suspend. Since the locking is done in background it opens up for a race with pm-suspend, which is faster nowadays.

Simply removing the backgrounding "&"'s in /usr/share/acpi-support/screenblank solves the problem. The suspend process will now take a bit longer of course, but resuming is faster.

Tormod,

There is no backgrounding of the xscreensaver-command in the /usr/share/acpi-support/screenblank that we ship:

if [ `pidof xscreensaver` ]; then
        su $user -c "(xscreensaver-command -throttle)"
                if [ x$LOCK_SCREEN = xtrue ]; then
                su $user -c "(xscreensaver-command -lock)"
        fi

I don't see any bug in acpi-support. Can you please check that you're using an unmodified package?

Oh sorry, I thought Ubuntu was using the same Debian patches. Well there must be another reason for this issue in Ubuntu then. I'll move the status back to "Confirmed", it was not me who confirmed it.

Is it acpi-support that calls xscreensaver-command on a standard installation of Xubuntu 12.10? I don't even have acpi-support on my Lubuntu 12.04.

> Is it acpi-support that calls xscreensaver-command on a standard installation of Xubuntu 12.10?

I have no idea. I would have expected it to be xfce4-power-manager, given that acpi-support was extended to recognize said program in response to bug #425155; but xfce4-power-manager appears to no longer be on the Xubuntu image.

Xfce runs xflock4 to lock the session. It is a shell script which gets installed by the package xfce4-session.

This bug is younger than Bug #423930 and apparently this bug was also originally reported against xscreensaver. If this report is still handling a bug of xscreensaver, it should be marked as a duplicate of Bug #423930.

However, the end-user problem can be dealt by ensuring that "xscreensaver-command -lock" will be finished before suspending. That is done by running the xscreensaver-command and the possible script enclosing it on foreground and by letting them run succesfully before entering suspend mode. Thus the fix happens in the calling code, not necessarily in xscreensaver itself. For xfce4-session the calls of xflock4 script should be fixed, for lxsession, the calls of lxlock should be fixed, and xfce4-power-manager's call of xscreensaver-command might need fixing, too. As for xflock4 and lxlock, some alternative locking utilities, such as slock, should be run on background to let the script finish before user has unlocked the lock, so that e.g. suspend mode can be entered while session is locked.

Jarno, I think it is OK that the older bug was marked as a duplicate because this newer one has more information and developer activity. The bug is in xfce4-session, not in xscreensaver.

Bug #1229486 is even newer, and has even more developer activity, and is targeted for xfce4-session and xscreensaver. Bug #423930 can not be marked as a duplicate on that one, if it is marked as a duplicate on this one, right?

Status changed to 'Confirmed' because the bug affects multiple users.

Any news on this issue? The bug was originally reported against xscreensaver, and it has been fixed in utopic (bug 1229486).

Yes, but the race condition still exists, if the system does not wait until screen has been locked before suspending.

I just added Unity to this bug. I looked for a way to report this directly to the Unity project, but it wasn't immediately apparent.

I'm seeing identical behaviour using stock unity on Ubuntu 15.10.

Jarno, does the race condition still exist in 18.04? If yes, what changes are needed to finally fix this issue?

Theo, Xfce Bugzilla #10089 should be fixed.

Testing this is easiest with xscreensaver:

$ pkill light-locker (or whatever screen saver daemon is running.)
$ xscreensaver
(Install it first, if you have to.)

Setup long "fade to black when blancking" duration in the Advanced tab of Screensaver Preferences. Make sure lock when going to sleep is chosen in the security settings of Xfce4 power manager.

Suspend and resume

If desktop is shown some time before asking password, the bug exists.

That said, light-locker seems to be the only locker that Bug #904006 does not occur with (in Xubuntu). light-locker does not show desktop that long time, I suppose.

Jarno, would you be able to contribute a patch to improve the functionality?

This is a long-standing issue and still not fully fixed. The upstream developers may not be aware of its severity.

Theo, sorry, but I can not even find the part of code where the locking is executed as part of suspend to RAM function.

The relevant code should be in the following two files:

xfce4-session/xfsm-systemd.c
xfce4-session/xfce-screensaver.c

Oh, yes, albeit the xfce-screensaver.c is present only in newer xfce4-session than what is used by Xubuntu.

Please test this patch, if you are able to build xfce4-session. (I created the patch using diff -Nrud)

The attachment "call xflock4 synchronously and return according to its exit status" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

What if the locker command does not exit immediately (waits until unlock)?

That is handled in xflock4 I attached in https://bugzilla.xfce.org/show_bug.cgi?id=10217 :

If such a fallback locker is used, they will be called in background in xflock4. If you have defined such a locker in xfce4-session -> /general/LockCommand configuration variable, you should not set xfce4-session -> /general/LockWaitComplete to true. Otherwise, when you suspend (with expected locking) it will just lock, but suspends only after you unlock. If xflock4 expects a locker not to return before unlock, it will be run in background and its success is determined by checking, if it is still running after short delay.

Alright. I see that you have covered most use cases while turning xflock4 into a rather complex shell script.

This needs to be approved upstream, but I am not sure if anything will happen..

"The thing is that xflock4 is not fixable, it's a hack that's broken by design and needs to go away."

Why does it need to be approved upstream?

You asked what if the locker does not exit immediately. It is pointless question with current xflock4 (in Xubuntu releases), as it does not have such lockers (or they are run in background). xlock is not available for Ubuntu. slock is so tiny and fast locker that there is no issue with it.

The more complex xflock4 has more features, if desired. Those features can be easily backported to Xubuntu, if desired. I suppose the xflock4 does not need to be approved upstream. In Xubuntu xflock4 has line "light-locker-command --lock \", but it has newer been accepted upstream.

So have you been able to test my patch you asked for?

The approval by upstream was mainly meant for xflock4. Also, I assumed that /general/LockCommand is already included in 4.12.1, but it is not.

Your patch for xfsm-systemd.c appears to have the expected effect. However, it breaks suspend in my test environment.

Oh, how does it break suspend?

This error is logged in .xsession-errors:

(xfsm-shutdown-helper:28111): GLib-WARNING **: 14:20:07.868: GError set over the top of a previous GError or uninitialized memory.
This indicates a bug in someone's code. You must ensure an error is NULL before it's set.
The overwriting error message was: Failed to execute child process ?/usr/sbin/pm-suspend? (No such file or directory)

You could try replacing
"g_spawn_command_line_sync ("xflock4", NULL, NULL, exit_status, error);"
by
"g_spawn_command_line_sync ("xflock4", NULL, NULL, exit_status, NULL);"

(In git in calls of g_spawn_command_line_async NULL is used for the error parameter (in xfce-sreensaver.c). Error variable might have some additional information about the error, though.

Still not working. Oddly, the log entry is from xfsm-shutdown-helper.

After disabling 'lock on suspend' I can suspend just fine.

Do you have pm-utils installed? What if you use another locker in xflock4?

The package pm-utils is not installed. It happens with any screen locker.

This is tested in a live session environment inside a VM with the xfce4-session package rebuilt to include your patch.

Oh, it seems to be dropped from Xubuntu.

How did you launch suspend? Try "xfce4-session-logout --suspend" in terminal.

xfsm-shutdown-helper seems still to rely on pm-utils on Linux.

I suspend the system via xfce4-session-logout > suspend.

It works with the default xfce4-session package. It works if I rebuild the package with no changes. The problem only occurs if I rebuild the package with your patch added to the Debian patch series.

I have asked bluesabre from the Xubuntu team to test your patch too.

Maybe it would work in Xenial that has pm-utils.

The "error" you tell in #47 is actually not an error, but warning. It should not stop suspend from working. Theo, did bluesabre test the patch? Did the test fail even if xflock4 returned 0?

I've tried this patch in Xubuntu 18.10, and my desktop is still briefly visible when coming back from suspend. I do believe the amount of time it is visible is reduced, but it's still visible nonetheless.

Just some additional thoughts brought to you by the work of the fella's over at the LXQt project on this same issue:

TL;DR: systemd-logind may not be inhibiting long enough for screen-lockers to complete. Variable adjustments can be made.

1. https://github.com/lxqt/lxqt/issues/1515#issuecomment-425371720

and

2. https://github.com/lxqt/lxqt/issues/1515#issuecomment-435624731

Sean, I think then there is a bug in the locker of your choice (which?), if it does not hide the screen before its command exits. Anyway, I am glad to know that you could use the patch in Xubuntu 18.10.

Jarno, just from looking at it, I think your patch is wrong.
You passed an uninitialized pointer to g_spawn_command_line_sync().
This function will write to whatever address was passed. That's a memory corruption and probably the source of the error message in comment #47.

Please try the attached patch.

Why didn't the compiler warn about the uninitialized variable? Don't you use -Wuninitialized (or -Wall for developing)?

My first corrected patch is still not checking the return value of g_spawn_command_line_sync().
https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#g-spawn-command-line-sync
Fixed this now.

Again, I don't have the source checked out, so I _manually_ edited the patch. It may not apply cleanly.

Note for the upstream bug[1], that this function has changed in master. It now calls
https://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfce-screensaver.c#n484.
That function still has that bug in the SCREENSAVER_TYPE_OTHER case.
The fix should look similar to what I attached. I might write a patch in a few weeks if you don't beat me to it. :)

[1]: https://bugzilla.xfce.org/show_bug.cgi?id=10089

Martin Castillo (2-acc-von), oh I thought the function would reserve some memory for the variable. I do not have the source checked out either, but maybe later. Thanks for your contribution.

This is an untested patch for xfce 4.14.

This is an untested patch for 4.12 and maybe earlier versions.