Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Y2038: query systemd-logind instead of utmp for username and tty (#2088) #2100

Merged
merged 3 commits into from Aug 16, 2023

Conversation

thkukuk
Copy link
Contributor

@thkukuk thkukuk commented Mar 3, 2023

Three out of four, lslogins.c will take longer as that is much more work.
sd_session_get_username() will only be available with systemd v254, but it builds and works with current git version of systemd already.

@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 3, 2023

Ok, need to find out why the test succeeds even if the function does not exist in the test environment...

@thkukuk thkukuk force-pushed the logind branch 2 times, most recently from 71eb1ad to 657d814 Compare March 4, 2023 11:35
@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 5, 2023

This one works for me and passes all tests. Would it be fine to get merged?

term-utils/wall.c Outdated Show resolved Hide resolved
term-utils/wall.c Outdated Show resolved Hide resolved
term-utils/write.c Outdated Show resolved Hide resolved
@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 8, 2023

@t-8ch I hope this is now Ok so.

@kilobyte
Copy link

kilobyte commented Mar 9, 2023

This would add a {lib,}systemd dependency, which is quite a regression.

I don't see what's the problem: the on-disk format doesn't need to match the API, and unlike a lot other uses of time_t, it's impossible for logs to include relevant pre-1970 dates.

@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 9, 2023

This would add a {lib,}systemd dependency, which is quite a regression.

util-linux has already a {lib,}systemd dependency if you configure --with-systemd, if you don't, this PR doesn't change anything.

I don't see what's the problem: the on-disk format doesn't need to match the API, and unlike a lot other uses of time_t, it's impossible for logs to include relevant pre-1970 dates.

The problem is Y2038 and that this affects the ABI: https://www.thkukuk.de/blog/Y2038_glibc_utmp_64bit/

@kilobyte
Copy link

kilobyte commented Mar 9, 2023

Your patch uses libsystemd instead of utmp, which make it fail to obtain the information on any distros that support systemd and either modular inits or init-less containers.

As for the ABI — save than possibly some C++ symbol decoration, the ABI is identical for int32_t and uint32_t.

@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 9, 2023

Your patch uses libsystemd instead of utmp, which make it fail to obtain the information on any distros that support systemd and either modular inits or init-less containers.

Please read the blog, everything is already answered there. glibc developers decided that they will deprecate and drop utmp because of the Y2038 problem and many other issues. Containers don't have, use nor support utmp at all. So this doesn't matter. Tools like agetty, write and wall don't work in containers anyways, too.
musl libc e.g. doesn't support utmp/utmpx at all, else there is still e.g. utmps which you can use. This patch does not remove the utmp code, but allows systemd distros to make use of systemd-logind. Or systemd-less distros, to make use of elogind. But utmps in parallel to systemd or elogind doesn't make much sense.
So please make yourself at first familiar with all the backgrounds.

As for the ABI — save than possibly some C++ symbol decoration, the ABI is identical for int32_t and uint32_t.

C++ symbol decoration with glibc? Tell that the glibc developers. Many more people will be very happy if you solve that problem, because this is the biggest issue with 64bit time_t on 32bit architectures currently.

@karelzak
Copy link
Collaborator

karelzak commented Mar 9, 2023

Note that I'll postpone this to the release v2.40, for v2.39 it's too late for these changes. I guess it's nothing urgent.

@karelzak
Copy link
Collaborator

karelzak commented Mar 9, 2023

For now (for some transitional period), it would probably be better to provide a way how to compile util-linux with systemd support, but also with traditional utmp.

It means hiding the new feature under a new build system option (--with-systemd-utmp or something better).

I think "utmp is dead" is a significant change and we should provide distros and admins time to adopt it. I don't think that everyone who uses systemd wants (or is ready) to kill utmp.

@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 9, 2023

Note that I'll postpone this to the release v2.40, for v2.39 it's too late for these changes. I guess it's nothing urgent.

No problem.

For now (for some transitional period), it would probably be better to provide a way how to compile util-linux with systemd support, but also with traditional utmp.

It means hiding the new feature under a new build system option (--with-systemd-utmp or something better).

What we can also do:
If "--with-systemd" is used, try at first systemd and if that reports as error "ENOENT", fallback to the utmp code.
And later add an option "--disable-utmp" to remove this fallback and creating utmp records in login and similar places.
Or add "--with-systemd-logind" and change "USE_SYSTEMD" to "USE_LOGIND". But this will be really ugly in configure.ac.
I can implement whatever you prefer.

@thkukuk
Copy link
Contributor Author

thkukuk commented Mar 27, 2023

What we can also do: If "--with-systemd" is used, try at first systemd and if that reports as error "ENOENT", fallback to the utmp code.

I have to correct me, this is not possible: if libsystemd functions are called and no systemd-logind is runnin, it does not report "ENOENT" or any other error back to the caller, it reports success with empty data. So there is no way to differentiate, if the systemd-logind is not running or e.g. nobody is logged in into the system.

@bluca
Copy link
Member

bluca commented May 29, 2023

For now (for some transitional period), it would probably be better to provide a way how to compile util-linux with systemd support, but also with traditional utmp.

It means hiding the new feature under a new build system option (--with-systemd-utmp or something better).

I think "utmp is dead" is a significant change and we should provide distros and admins time to adopt it. I don't think that everyone who uses systemd wants (or is ready) to kill utmp.

I wouldn't bother with that tbh. We should be preparing for the new systemd release soon, so it should be out before the util-linux release containing this change, so the new API should be available in places where the new util-linux will be picked up.

@kilobyte
Copy link

That'd be the nicest option, yeah. If systemd allows us to tell apart "no systemd" from "no data", then utmp code can continue working with no regressions.

@bluca
Copy link
Member

bluca commented May 29, 2023

What we can also do: If "--with-systemd" is used, try at first systemd and if that reports as error "ENOENT", fallback to the utmp code.

I have to correct me, this is not possible: if libsystemd functions are called and no systemd-logind is runnin, it does not report "ENOENT" or any other error back to the caller, it reports success with empty data. So there is no way to differentiate, if the systemd-logind is not running or e.g. nobody is logged in into the system.

The canonical way of checking whether you are running in a systemd system is to check for the existence of the /run/systemd/ directory, you could do that before calling the sd API? Changing the return codes is an API breakage and we are generally extremely careful about that. We could consider it if absolutely necessary, probably with new interfaces, but only if there are no other solutions.

@thkukuk
Copy link
Contributor Author

thkukuk commented May 30, 2023

Ok, changed the code: if system was booted with systemd I will use the systemd-logind, else the code will fallback to utmp.

term-utils/agetty.c Fixed Show resolved Hide resolved
@bluca
Copy link
Member

bluca commented May 30, 2023

Ok, changed the code: if system was booted with systemd I will use the systemd-logind, else the code will fallback to utmp.

I spoke with other maintainers, we'd actually be fine with a PR that changes those APIs to return a precise error code in case we detect that we are not running under systemd (but making sure the running-in-chroot case still returns 0 as that's a separate workflow that is still valid and should say 'no sessions'). The patch should be super easy as we already have internal helpers to check both cases, so should be a couple of lines.

@thkukuk
Copy link
Contributor Author

thkukuk commented May 30, 2023

Ok, changed the code: if system was booted with systemd I will use the systemd-logind, else the code will fallback to utmp.

I spoke with other maintainers, we'd actually be fine with a PR that changes those APIs to return a precise error code in case we detect that we are not running under systemd (but making sure the running-in-chroot case still returns 0 as that's a separate workflow that is still valid and should say 'no sessions'). The patch should be super easy as we already have internal helpers to check both cases, so should be a couple of lines.

Thanks, but since there is already sd_booted(), I'm not sure if this is necessary.

term-utils/agetty.c Outdated Show resolved Hide resolved
@kilobyte
Copy link

On an error, it'd be better to use utmp. It might or might not be available — so either you get a good answer after all, or not but no loss.

@bluca
Copy link
Member

bluca commented May 30, 2023

Only when not booting on systemd

@thkukuk
Copy link
Contributor Author

thkukuk commented May 31, 2023

On an error, it'd be better to use utmp. It might or might not be available — so either you get a good answer after all, or not but no loss.

This requires that there is no "legacy" application left writing additional utmp entries. If such an application is left, you will get wrong results in this case.
Since this only affects agetty, and is only a purely informal message, I would just report "zero" if systemd is used and an error occured.

@bluca
Copy link
Member

bluca commented May 31, 2023

On an error, it'd be better to use utmp. It might or might not be available — so either you get a good answer after all, or not but no loss.

This requires that there is no "legacy" application left writing additional utmp entries. If such an application is left, you will get wrong results in this case. Since this only affects agetty, and is only a purely informal message, I would just report "zero" if systemd is used and an error occured.

That sounds like a sensible approach to me

term-utils/agetty.c Outdated Show resolved Hide resolved
@bluca
Copy link
Member

bluca commented Aug 14, 2023

@karelzak anything left to do here? Looks ready to me

@karelzak karelzak merged commit 8032ccb into util-linux:master Aug 16, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants