Code review comment for lp:~xnox/upstart/user-log-dir

On Thu, Jan 17, 2013 at 02:20:29PM -0000, James Hunt wrote:

> > The spec is unclear about XDG_RUNTIME_DIR, I'd like to assert if it
> > doesn't exist. In ubuntu it exists in quantal and up. To be compliant
> > with the spec we should gracefully fallback to some other place, but I
> > don't know what that other place is a good to fallback to.

> Asserting is a little harsh. If it's not set, how about we display a
> warning and then fall back to ${TMPDIR}/${USER} and if that's not set,
> /tmp/${USER}? Steve

I suggest consulting the desktop team, or looking at existing fd.o software,
for recommendations on a fallback path. Using ${TMPDIR:-/tmp}/${USER} has
some obvious security problems (best case: DoS) that are undesirable even in
a fallback.

I do agree that an assert() here is a bit harsh. In theory we should never
be without $XDG_RUNTIME_DIR in quantal and beyond; however, this is
implemented by way of the pam stack and relies on the local admin not
altering /etc/pam.d/common-session in an "unsupported" manner. I fear that
having upstart assert instead of opening a login session will be rather
undebuggable, and I doubt this is a good tradeoff for something that's
rather peripheral to the main functionality of upstart.

> > I believe XDG_RUNTIME_DIR is cleaned up for us after the last logout.
> > I'll check that. Or do you mean to clean up bits in the
> > $XDG_RUNTIME_DIR/upstart/ that we created? E.g. remove pidfiles on
> > exit?

> What is doing the cleaning up currently? And will that need to change when
> Upstart is in control?

pam_xdg_support, which is the same thing that creates the directory and sets
the env variable. This must not change under upstart user sessions.