! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
sudo xfsm-shutdown-helper fallback when HAL is not available
Status:
RESOLVED: FIXED
Product:
Xfce4-session
Component:
General

Comments

Description Landry Breuil editbugs 2009-01-23 23:41:51 CET
I know, HAL is everywhere, and sudo fallback is considered unmaintained. Although having it not working fucks up OpenBSD/NetBSD/Solaris users, and maybe other linux distribs (i know slackware provides HAL only since its last version,.)

So i dig in its code, and here are my findings :

- xfce4-session-logout --reboot only logs you out (ditto when clicking on reboot in window)
- unrelated, but the code in xfsm-shutdown-helper.c used to ask password to the user seems unused. When you don't have NOPASSWD set in sudoers, only LogOut option is available, others are greyed. Maybe those chunks of code can be removed/marked deprecated
- echo 'REBOOT' | sudo /usr/local/libexec/xfsm-shutdown-helper does its job. So xfce4-session doesn't send him the good command -> bingo.

There's an offset mistake in xfsm-shutdown-helper.c:	xfsm_shutdown_helper_send_command(), introduced in r28049. it receives command, which is now the enum defined in shutdown.h, 1=logout, 2=halt, 3=reboot.. but command_table[command] is wrong, as 0='POWEROFF' and 1='REBOOT'. (which previously corresponded to XFSM_SHUTDOWN_POWEROFF = 0 / XFSM_SHUTDOWN_REBOOT = 1 in xfsm-shutdown-helper.h)

The simplest fix (to me) is to have command_table[command - 2], or this dumb snippet :

if (command == XFSM_SHUTDOWN_HALT)
  fprintf (helper->outfile, "POWEROFF\n");
else
  fprintf (helper->outfile, "REBOOT\n");

At some point, suspend/resume can be supported in this sudo fallback too (as it was done previously), no need to _only_ rely on HAL. If needed, i can take care of the maintenance of these parts..

As a side note, you added some error reporting through dialogs.. it sometimes happen to me to have (after session is saved and all windows closed) 'Error receiving response from shutdown helper: interrupted system call' dialog.. which may make sense, as the system is shutting down, there might be a race condition with the helper sending back 'SUCCEED', and fgets() in xfce4-session.

Thoughs ? Is my reasoning correct ?
Comment 1 Brian J. Tarricone (not reading bugmail) 2009-01-23 23:57:44 CET
(In reply to comment #0)

> There's an offset mistake in xfsm-shutdown-helper.c:   
> xfsm_shutdown_helper_send_command(), introduced in r28049. it receives command,
> which is now the enum defined in shutdown.h, 1=logout, 2=halt, 3=reboot.. but
> command_table[command] is wrong, as 0='POWEROFF' and 1='REBOOT'. (which
> previously corresponded to XFSM_SHUTDOWN_POWEROFF = 0 / XFSM_SHUTDOWN_REBOOT =
> 1 in xfsm-shutdown-helper.h)
> 
> The simplest fix (to me) is to have command_table[command - 2]

Do you have a patch?

> At some point, suspend/resume can be supported in this sudo fallback too (as it
> was done previously), no need to _only_ rely on HAL. If needed, i can take care
> of the maintenance of these parts..

Yeah, that would be fine, for 4.8.

> As a side note, you added some error reporting through dialogs.. it sometimes
> happen to me to have (after session is saved and all windows closed) 'Error
> receiving response from shutdown helper: interrupted system call' dialog..
> which may make sense, as the system is shutting down, there might be a race
> condition with the helper sending back 'SUCCEED', and fgets() in xfce4-session.
> 
> Thoughs ? Is my reasoning correct ?

Hmm, yeah, that could be the problem.  It's probably safe to ignore EINTR at that point.
Comment 2 Landry Breuil editbugs 2009-01-24 11:02:07 CET
Created attachment 2110 
Get the right offset in command_table, and ignore EINTR

Here you are, fixes the issue for me.
Comment 3 Landry Breuil editbugs 2009-02-01 21:41:32 CET
Created attachment 2141 
Fixes sudo fallback helper

whoops, wrong patch.
Comment 4 Brian J. Tarricone (not reading bugmail) 2009-02-02 06:12:09 CET
Committed, thanks.  (In the future, please, one bug report and patch per issue.)
Comment 5 Landry Breuil editbugs 2009-09-24 19:17:53 CEST
A crash still happens in this fix (commit 588bcec67ca5aba6a91905ce589cce2283c5a9e5) :

if fgets() or ferror() fail in xfsm-shutdown-helper.c:xfsm_shutdown_helper_send_command() and errno is set to EINTR, false is returned but error is not set.

Then, in shutdown.c:xfsm_shutdown(), xfce_message_dialog() is called with error->message, causing a NULL dereference.

Two options to fix this crash :

- in xfsm_shutdown(), call xfce_message_dialog() only if error is set. quite ugly.
- in xfsm_shutdown_helper_send_command(), move the 'return FALSE' statement _inside_ the check for 'error && errno != EINTR'.

I'll tend to prefer the latter.
Comment 6 Brian J. Tarricone (not reading bugmail) 2009-09-24 21:11:20 CEST
Ok, ended up just returning true on the spot if it's EINTR.  Pushed to master and 4.6 branch.
Comment 7 Brian J. Tarricone (not reading bugmail) 2009-09-24 21:11:55 CEST
er, mark fixed again.

Bug #4849

Reported by:
Landry Breuil
Reported on: 2009-01-23
Last modified on: 2009-09-24

People

Assignee:
Brian J. Tarricone (not reading bugmail)
CC List:
0 users

Version

Version:
4.5.93 (4.6 beta 3)

Attachments

Additional information