Export dbus address conditionally #11378
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keszybz
force-pushed
the
export-dbus-address-conditionally
branch
from
12d5118
to
b795ddc
Compare
mrc0mmand
reviewed
Jan 10, 2019
keszybz
force-pushed
the
export-dbus-address-conditionally
branch
from
b795ddc
to
e6cee47
Compare
|
This pull request introduces 1 alert when merging e6cee47 into b26c904 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
Same as all the other PRs filed today. |
poettering
reviewed
Jan 15, 2019
src/login/pam_systemd.c
Outdated
| * daemons that spawn dbus-daemon, instead of forcing | ||
| * DBUS_SESSION_BUS_ADDRESS= here. */ | ||
|
|
||
| s = strjoin(runtime, "/bus"); |
poettering
reviewed
Jan 15, 2019
src/login/pam_systemd.c
Outdated
| @@ -199,6 +199,18 @@ static int export_legacy_dbus_address( | |||
| _cleanup_free_ char *s = NULL; | |||
| int r = PAM_BUF_ERR; | |||
|
|
|||
| /* FIXME: We *really* should move the access() check into the | |||
| * daemons that spawn dbus-daemon, instead of forcing | |||
| * DBUS_SESSION_BUS_ADDRESS= here. */ | |||
There was a problem hiding this comment.
i think we should conditionalize this as LEGACY or so? I don't think this is really worth supporting in new stuff
There was a problem hiding this comment.
I prefer not to make things more complicated than they need to be. The tribulations we went through with systemd-240 suggest that dropping this is going to have bad effects even on recently written software. I think it's OK to export the variable without any plans to remove it.
poettering
reviewed
Jan 15, 2019
src/login/pam_systemd.c
Outdated
|
|
||
| if (asprintf(&rt, "/run/user/"UID_FMT, pw->pw_uid) < 0) | ||
| return PAM_BUF_ERR; | ||
| assert_cc(sizeof(uid_t) <= sizeof(int)); |
There was a problem hiding this comment.
why the assert? seems unnecessary to check this?
|
if we readd the access() check it really have a clear explanation what the usecase for this is in an adjacent comment |
This reverts commit 69bd76f. $DBUS_SESSION_BUS_ADDRESS is again set only if the socket exists. Quoting systemd#11327 (comment): > [setting $DBUS_SESSION_BUS_ADDRESS unconditionally] makes pam_systemd > incompatible with installations and distributions where dbus was not > configured with --enable-user-session, and the session dbus-daemon is started > by autolaunching or dbus-launch (as opposed to dbus.socket). I don't think > that's wise: using autolaunching or dbus-launch, and disabling or not > installing dbus.socket and dbus.service on the systemd user instance, is our > compatibility story for people who still need a D-Bus session bus per X11 > session for whatever reason. > > For example, Debian can currently do either way, with a dbus-user-session > package strongly recommended but not actually mandatory. dbus-user-session > requires libpam-systemd; if pam_systemd now requires dbus.socket (which is in > the dbus-user-session package), that's a circular dependency, which we > normally try hard to avoid. For systems that use dbus.socket this doesn't matter much, because the user session is ordered after the user managaer, which pulls in dbus.socket very early. For example, when logging over ssh: sshd[20796]: pam_systemd(sshd:session): pam-systemd initializing sshd[20796]: pam_systemd(sshd:session): Asking logind to create session: uid=1001 pid=20796 service=sshd type=tty class=user desktop= seat= vtnr=0 tty= display= remote=yes remote_user= remote_host=::1 sshd[20796]: pam_systemd(sshd:session): Session limits: memory_max=n/a tasks_max=n/a cpu_weight=n/a io_weight=n/a systemd[1]: Created slice User Slice of UID 1001. systemd[1]: Starting User Runtime Directory /run/user/1001... systemd-logind[1210]: New session 3796 of user guest. systemd[1]: Started User Runtime Directory /run/user/1001. systemd[1]: Starting User Manager for UID 1001... systemd[20805]: pam_systemd(systemd-user:session): pam-systemd initializing systemd[20805]: Starting D-Bus User Message Bus Socket. ... systemd[20805]: Reached target Sockets. systemd[20805]: Reached target Basic System. systemd[1]: Started User Manager for UID 1001. systemd[1]: Started Session 3796 of user guest. sshd[20796]: pam_systemd(sshd:session): Reply from logind: id=3796 object_path=/org/freedesktop/login1/session/_33796 runtime_path=/run/user/1001 session_fd=13 seat= vtnr=0 original_uid=1001 sshd[20796]: pam_unix(sshd:session): session opened for user guest by (uid=0) Hence, everything in the ssh session is ordered after the user instance. And in the user instance, services should be orderd after dbus.socket using inter-unit dependencies. dbus.socket in turns does systemctl --user set-environment DBUS_SESSION_BUS_ADDRESS=unix:path=%t/bus. So there should be no race between starting of the dbus socket and our check if it exists. The alternative would be to set the "DBUS_SESSION_BUS_ADDRESS=unix:path=%s/bus;autolaunch:". AFAICT, this would work as well. But I don't see any case where it actually works better. Since this is an area with many compatiblity concerns, let's stick to the previous setup which seems to work well.
keszybz
force-pushed
the
export-dbus-address-conditionally
branch
from
e6cee47
to
15ee6c2
Compare
|
Updated. All comments are addressed except for the LEGACY check, see above. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixup for #11327. This takes the safer (I hope) route of reverting to the previous code.