Xsession.d script assumes that /sbin is in $PATH

Bug #1435492 reported by Johannes Mueller on 2015-03-23
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
upstart (Ubuntu)
Medium
Unassigned

Bug Description

According to changelog.Debian from version 1.12.3 on lightdm is using "bash for the session to improve error handling" to fix #678421.

Unfortunately this can break the session start up when the user sets the $PATH environment variable in ~/.bashrc. Then /sbin might no longer be in $PATH and /etc/X11/Xsession.d/99cadence-session-start won't find /sbin/upstart when calling "upstart --user"

The mechanism is the following:

1. /usr/sbin/lightdm-session lines 37-42 source among others ~/.profile

2. The usual .profile of /etc/skel will source ~/.bashrc if the shell is Bash (and not /bin/sh)

3. ~/.bashrc (maybe) is setting $PATH without /sbin

4. upstart --user call fails as /sbin is not in $PATH

Suggested solutions:

* switch back to /bin/sh

* make sure that /sbin is in $PATH

Related branches

description: updated
description: updated
Gunnar Hjalmarsson (gunnarhj) wrote :

Hi Johannes, thanks for your report.

I'm the one who proposed the use of /bin/bash, so I'm kind of disinclined to agree on switching back. ;) There were valid reasons for making that change.

Can't help thinking that you ask for trouble by excluding /sbin from $PATH, but possibly it's not just a mistake.

Other ways, besides those you mention, to deal with this bug might be:

* Change the upstart files so they call the upstart bin with full path.

* Add some condition to the .profile file in /etc/skel so it does not
  source ~/.bashrc when sourced by a DM.
  ( Got that idea from http://askubuntu.com/questions/591937 )

Subscribed Martin Pitt, who helped out with bug #678421.

Johannes Mueller (johmue) wrote :

When I was young I've been taught that the sbin dirs are not supposed to be in the user's $PATH, but only in the superuser's. (the "s" in /sbin stands for "superuser"). Usually in the sbin dirs are only tools you need for sysadmin tasks. So it is a good idea not to have it in the $PATH of an ordinary user.

I also doubt a little bit about changing /etc/skel/.profile as this would only affect new added users. All the already existing user accounts have their ~/.profile in their $HOME. That wouldn't be affected by changing /etc/skel/.profile

A solution I just tested is to unset BASH_VERSION in /usr/sbin/lightdm-session. Then standard ~/.profile wouldn't source .bashrc. I don't know if this has side effect though.

tags: added: patch
Martin Pitt (pitti) wrote :

IMHO the simplest fix would just be to call it with full path, i. e. /sbin/upstart --user ... ?

Gunnar Hjalmarsson (gunnarhj) wrote :

On 2015-03-27 13:54, Martin Pitt wrote:
> IMHO the simplest fix would just be to call it with full path, i. e.
> /sbin/upstart --user ... ?

Ok.. Then, to address the OP's issue, we are talking about fixing upstart. Should we change the affected package to upstart?

Martin Pitt (pitti) wrote :

Yes, I agree.

affects: lightdm (Ubuntu) → upstart (Ubuntu)
Changed in upstart (Ubuntu):
status: New → Triaged
summary: - Xsession crash after login (fix for #678421 breaks)
+ Xsession.d script assumes that /sbin is in $PATH
Gunnar Hjalmarsson (gunnarhj) wrote :
Gunnar Hjalmarsson (gunnarhj) wrote :

I have attached a new diff file for upstart together with a patch which shows the delta. This takes care of the potiential issue as far as upstart is concerned.

However...

@Johannes: /etc/X11/Xsession.d/99cadence-session-start does not belong to the upstart package, so this does not really fix your specific situation. Actually I don't see the cadence package anywhere in the Ubuntu archive. To fix it for yourself, you may want to edit that file, so it uses the full path when calling upstart. Preventing ~/.profile from sourcing ~/.bashrc is another way. (Any manual edit of /usr/sbin/lightdm-session will get lost at the next update of the lightdm package.)

Changed in upstart (Ubuntu):
assignee: nobody → Gunnar Hjalmarsson (gunnarhj)
status: Triaged → In Progress
importance: Undecided → Medium
Didier Roche (didrocks) wrote :

Please post a new debdiff as we discussed on IRC, thanks! (setting to incomplete to remove from the list meanwhile)

Changed in upstart (Ubuntu):
status: In Progress → Incomplete
Gunnar Hjalmarsson (gunnarhj) wrote :

Corrected patch attached.

Changed in upstart (Ubuntu):
status: Incomplete → In Progress
Didier Roche (didrocks) on 2015-04-03
Changed in upstart (Ubuntu):
status: In Progress → Fix Committed
Johannes Mueller (johmue) wrote :

The patch in #9 doesn't fix it completely for me, as also some of the scripts in /usr/share/upstart/sessions/* assume that /sbin is in $PATH. Some of them call initctl which resides in /sbin

Unfortunately this means that a lot of packages would need to be fixed.

A grep->loop->dpkg -S->awk->sort->uniq command gave on my machine the following list of affected packages.

avahi-daemon
cgmanager
dbus
gnome-keyring
gnupg-agent
indicator-application
kde-workspace-bin
logrotate
openssh-client
rsync
upstart-bin

Sorry for being that late about this. I only had time now to test and debug it.

In my opinion it's upstart's fault to put executables that are needed by non-root users into (/usr)?/sbin. That is against what I learned back in the 1990 when I started using UNIX / Linux.

Gunnar Hjalmarsson (gunnarhj) wrote :

Hi Johannes,

My own feeling is that Ubuntu doesn't distinguish as strictly between bin and sbin as you would like to see. This is the standard $PATH in the default /etc/environment:

PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games"

Possibly it can be justified by the fact that the root user is not enabled by default in a fresh Ubuntu install, but you are encouraged to use sudo when needed. After all, running stuff in sbin as a normal user don't typically do much harm, right?

Anyway, I think that some experienced developer should better comment on your objection.

The easiest ways to fix it for yourself are to either not dropping /sbin from $PATH in ~/.bashrc or prevent ~/.profile from sourcing ~/.bashrc.

Gunnar Hjalmarsson (gunnarhj) wrote :

Brw, would this symlink help?

$ ll /usr/share/upstart/sessions/initctl
lrwxrwxrwx 1 root root 13 apr 4 02:30 /usr/share/upstart/sessions/initctl -> /sbin/initctl*

OTOH I suppose that '.' must be included in $PATH for that to work, and that's not recommended.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package upstart - 1.13.2-0ubuntu12

---------------
upstart (1.13.2-0ubuntu12) vivid; urgency=medium

  * debian/xsession.d/99upstart:
    Don't assume that /sbin is in $PATH when calling upstart
    (LP: #1435492).
 -- Gunnar Hjalmarsson <email address hidden> Fri, 03 Apr 2015 14:48:00 +0100

Changed in upstart (Ubuntu):
status: Fix Committed → Fix Released
Johannes Mueller (johmue) wrote :

Hi Gunnar,

maybe I missed that ubuntu does interpret the meaning of /sbin differently than the classic UNIX systems. In that case I'd fix it in my .bashrc.

I really would be interested in a statement of an experienced ubuntu developer about such a decission and the reasons for it.

Anyway I'm a bit surprised, that the "fix" was released that quickly, given that it does not fix anything. Either my point that one cannot assume that /sbin is in $PATH is right. Then the fix is not sufficient. Or it is wrong, then my whole bug report is not justified.

In the former case the logical solution would be, that no executables that a non root user needs should reside in an sbin dir. That would affect the package upstart-bin, that provides /sbin/upstart as well as /sbin/initctl. upstart-bin also has some executables in /bin, so there seems to be some distinction between /bin and /sbin

Now we have a fix for a maybe not justified bug report that actually does not solve the problem. I actually would be happier with "won't fix" and a clear statement that also (/usr)+/sbin should be in the user's $PATH.

Gunnar Hjalmarsson (gunnarhj) wrote :

Hi again, Johannes.

Admittedly you make a few valid points. To get an authoritative statement about bin vs. sbin, I would suggest that you post a message to the ubuntu-devel-discuss mailing list, possibly referring to this bug report.

https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel-discuss

I'm just a hobbyist contributor who make observations, and what I wrote in comment #11 is merely speculation. ;)

It's true that the uploaded patch does not fix the problem you brought up. Changing the status to "Confirmed" for now.

Changed in upstart (Ubuntu):
assignee: Gunnar Hjalmarsson (gunnarhj) → nobody
status: Fix Released → Confirmed
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments