shadow-maint / shadow

Upstream shadow tree
Other
304 stars 232 forks source link

login and logoutd broken on systems with utmps #945

Closed firasuke closed 8 months ago

firasuke commented 8 months ago

Hey there,

As of 4.14.0, login is no longer storing utmp entries on systems that use utmps (e.g. musl based Linux systems).

When using version 4.13, the output of w:

image

After this commit, w no longer shows any entry due to login not storing any.

What could be the issue here? Did shadow disable the entirety of utmpx even when using utmps (a secure implementation)?

Thank you for your time and effort on this.

firasuke commented 8 months ago

Apparently, prior to 4.14.0 this issue was solved by @awilfox at #118.

awilfox commented 8 months ago

We've pinned Adélie to 4.13 because of this, actually. Replacing utmp with utmpx and trying to convince the maintainers here to use that one instead of the older, legacy one (note that is standard because couldn't be standardized) is on my eternal backlog. If I dedicated some time to it, would that be mergable?

alejandro-colomar commented 8 months ago

We've pinned Adélie to 4.13 because of this, actually.

Sorry for the regression!

Replacing utmp with utmpx and trying to convince the maintainers here to use that one instead of the older, legacy one (note that is standard because couldn't be standardized) is on my eternal backlog.

Hmm, utmpx is a POSIX XSI extension. How portable is that? Maybe we can unconditionally use utmpx. That would probably be the simplest solution.

If I dedicated some time to it, would that be mergable?

Sure. :)

alejandro-colomar commented 8 months ago

I'm answering here to keep it on-topic.

This is the only remaining thing, I think:

* [login and logoutd broken on systems with utmps #945](https://github.com/shadow-maint/shadow/issues/945)

Cc: @firasuke, @awilfox

Perhaps this can be reverted.

That wouldn't be easy.

alx@debian:~/src/shadow/shadow/utmp$ git revert 170b76
Auto-merging configure.ac
CONFLICT (content): Merge conflict in configure.ac
Auto-merging lib/failure.c
CONFLICT (content): Merge conflict in lib/failure.c
Auto-merging lib/failure.h
CONFLICT (content): Merge conflict in lib/failure.h
Auto-merging lib/limits.c
CONFLICT (content): Merge conflict in lib/limits.c
Auto-merging lib/prototypes.h
CONFLICT (content): Merge conflict in lib/prototypes.h
Auto-merging lib/user_busy.c
Auto-merging lib/utmp.c
CONFLICT (content): Merge conflict in lib/utmp.c
Auto-merging src/login.c
CONFLICT (content): Merge conflict in src/login.c
Auto-merging src/logoutd.c
CONFLICT (content): Merge conflict in src/logoutd.c
error: could not revert 170b76cd... Disable utmpx permanently
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
alx@debian:~/src/shadow/shadow/utmp$ git diff --stat
 configure.ac     | Unmerged
 configure.ac     |  19 ++++++
 lib/failure.c    | Unmerged
 lib/failure.c    |  62 ++++++++++++++++++
 lib/failure.h    | Unmerged
 lib/failure.h    |  23 +++++++
 lib/limits.c     | Unmerged
 lib/limits.c     |  43 +++++++++++++
 lib/prototypes.h | Unmerged
 lib/prototypes.h |  27 ++++++++
 lib/utmp.c       | Unmerged
 lib/utmp.c       | 193 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/login.c      | Unmerged
 src/login.c      | 107 ++++++++++++++++++++++++++++++
 src/logoutd.c    | Unmerged
 src/logoutd.c    |  39 +++++++++--
 8 files changed, 508 insertions(+), 5 deletions(-)

Also, it would bring back many preprocessor conditionals, which I would like to avoid.

Or maybe have utmpx unconditionally available?

That's the easiest solution, I think, and since glibc claims that utmp and utmpx are identical, it should be easy. Just slightly more than s/utmp/utmpx/.

It would be even better if utmps was supported (being a secure implementation for musl based distros), in case you didn't want to bring the entirety of utmp.

I'll leave that as an improvement, but for fixing the regression I prefer to do something that is less of a change.

ikerexxe commented 8 months ago

It would be even better if utmps was supported (being a secure implementation for musl based distros), in case you didn't want to bring the entirety of utmp.

utmp and all the interfaces associated with it are broken, and there's no easy way of fixing it. https://www.thkukuk.de/blog/Y2038_glibc_utmp_64bit/ states the problem perfectly. Thus, I don't see this is as an option.

firasuke commented 8 months ago

It would be even better if utmps was supported (being a secure implementation for musl based distros), in case you didn't want to bring the entirety of utmp.

utmp and all the interfaces associated with it are broken, and there's no easy way of fixing it. https://www.thkukuk.de/blog/Y2038_glibc_utmp_64bit/ states the problem perfectly. Thus, I don't see this is as an option.

utmps utilizes a daemon similar to how logind works. Even Thorsten Kukuk (the author you cited) mentions utmps as a possible solution here.

alejandro-colomar commented 7 months ago

We've pinned Adélie to 4.13 because of this, actually. Replacing utmp with utmpx and trying to convince the maintainers here to use that one instead of the older, legacy one (note that is standard because couldn't be standardized) is on my eternal backlog. If I dedicated some time to it, would that be mergable?

@awilfox , I believe the problem you had has already been fixed, and you should be able to upgrade to 4.15 (in which case I'd recommend you to wait for 4.15.1 to be released on Monday https://github.com/shadow-maint/shadow/issues/968).