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

lib/, src/, configure.ac: Use utmpx instead of utmp #954

Merged
merged 4 commits into from
Feb 20, 2024

Conversation

alejandro-colomar
Copy link
Collaborator

utmpx is specified by POSIX as an XSI extension. That's more portable than utmp, which is unavailable for example in musl libc. The manual page specifies that in Linux (but it probably means in glibc), utmp and utmpx (and the functions that use them) are identical, so this commit shouldn't affect glibc systems.

Assume utmpx is always present.

Also, if utmpx is present, POSIX guarantees that some members exist:

  • ut_user
  • ut_id
  • ut_line
  • ut_pid
  • ut_type
  • ut_tv

So, rely on them unconditionally.

Fixes: 170b76c ("Disable utmpx permanently")
Closes: #945
Reported-by: @firasuke
Reported-by: @awilfox

lib/utmp.c Fixed Show resolved Hide resolved
@alejandro-colomar
Copy link
Collaborator Author

I suggest reviewing with

git show --color-words=. --patience

@alejandro-colomar
Copy link
Collaborator Author

v1b changes:

  • tfix subject line
$ git range-diff shadow/master gh/utmp utmp 
1:  24522aa0 ! 1:  af1b9f4d lib/, src. configure.ac: Use utmpx instead of utmp
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/, src. configure.ac: Use utmpx instead of utmp
    +    lib/, src/, configure.ac: Use utmpx instead of utmp
     
         utmpx is specified by POSIX as an XSI extension.  That's more portable
         than utmp, which is unavailable for example in musl libc.  The manual

@firasuke
Copy link

I will have to test this, though I don't see any changes being made to login.c?

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 17, 2024

I will have to test this,

Please, and thanks!

though I don't see any changes being made to login.c?

No, the changes are in the library code. login.c doesn't seem to be using utmp directly.

$ grep -rn utmp src/login.c
689:	 * information coming from login or from the caller (e.g. no utmp)
1178:	 * The utmp entry needs to be updated to indicate the new status
1181:	err = update_utmp (username, tty, hostname);
1183:		SYSLOG ((LOG_WARN, "Unable to update utmp entry for %s", username));

@awilfox , would you mind testing it too for your distribution?

@alejandro-colomar alejandro-colomar changed the title lib/, src. configure.ac: Use utmpx instead of utmp lib/, src/, configure.ac: Use utmpx instead of utmp Feb 17, 2024
@alejandro-colomar
Copy link
Collaborator Author

v2 changes:

  • s/endutent/endutxent/
$ git range-diff shadow/master gh/utmp utmp 
1:  af1b9f4d ! 1:  595c6909 lib/, src/, configure.ac: Use utmpx instead of utmp
    @@ lib/utmp.c: err_close:
                memcpy (ret, ut, sizeof (*ret));
        }
      
    -@@ lib/utmp.c: static
    +-  endutent ();
    ++  endutxent();
    + 
        return ret;
      }
      
    @@ src/logoutd.c: int main (int argc, char **argv)
                        if (ut->ut_type != USER_PROCESS) {
                                continue;
                        }
    +@@ src/logoutd.c: int main (int argc, char **argv)
    +                   exit (EXIT_SUCCESS);
    +           }
    + 
    +-          endutent ();
    ++          endutxent();
    + 
    + #ifndef DEBUG
    +           sleep (60);

@alejandro-colomar
Copy link
Collaborator Author

v3 changes:

  • Reduce scope of variable, to silence a warning.
$ git range-diff shadow/master gh/utmp utmp 
1:  595c6909 = 1:  595c6909 lib/, src/, configure.ac: Use utmpx instead of utmp
-:  -------- > 2:  e9255d0d lib/utmp.c: get_session_host(): Reduce scope of variable

@firasuke
Copy link

Ok I tried the utmp branch and I am receiving the following error when running ./configure:

...
checking for library containing readpassphrase... no
configure: error: readpassphrase() is missing, either from libc or libbsd
...

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 18, 2024

Ok I tried the utmp branch and I am receiving the following error when running ./configure:

...
checking for library containing readpassphrase... no
configure: error: readpassphrase() is missing, either from libc or libbsd
...

You need to either install libbsd, or autogen with --without-libbsd. I recommend using libbsd, but some systems, like Fedora, don't want to have libbsd in their core packages, so they use --without-libbsd.

See:

@firasuke
Copy link

Ok I tried the utmp branch and I am receiving the following error when running ./configure:

...
checking for library containing readpassphrase... no
configure: error: readpassphrase() is missing, either from libc or libbsd
...

You need to either install libbsd, or autogen with --without-libbsd. I recommend using libbsd, but some systems, like Fedora, don't want to have libbsd in their core packages, so they use --without-libbsd.

See:

* https://github.com/shadow-maint/shadow/blob/4d139ca466820b9a5fe8f75b9a0bee6a922d906a/share/containers/fedora.dockerfile#L13

* https://github.com/shadow-maint/shadow/blob/4d139ca466820b9a5fe8f75b9a0bee6a922d906a/share/containers/debian.dockerfile#L12

* https://github.com/shadow-maint/shadow/blob/4d139ca466820b9a5fe8f75b9a0bee6a922d906a/share/containers/alpine.dockerfile#L5

Silly me, I already had that disabled, but perhaps dropped it after downgrading. Thanks for the reminder!

@firasuke
Copy link

And I can confirm it is working as intended!

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Feb 18, 2024

v4 changes:

  • Tested-by: @firasuke
  • Don't use UT_LINESIZE with struct utmpx. (fixes CI dist-build)
$ git range-diff shadow/master gh/utmp utmp 
1:  595c6909 ! 1:  1d755860 lib/, src/, configure.ac: Use utmpx instead of utmp
    @@ Commit message
         Closes: <https://github.com/shadow-maint/shadow/issues/945>
         Reported-by: Firas Khalil Khana <firasuke@gmail.com>
         Reported-by: "A. Wilfox" <https://github.com/awilfox>
    +    Tested-by: Firas Khalil Khana <firasuke@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## configure.ac ##
2:  e9255d0d ! 2:  05e9aeb7 lib/utmp.c: get_session_host(): Reduce scope of variable
    @@ Commit message
     
         This silences a warning about an unused variable.
     
    +    Tested-by: Firas Khalil Khana <firasuke@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/utmp.c ##
-:  -------- > 3:  4628615a lib/sizeof.h: memberof(): Add macro
-:  -------- > 4:  64d45e98 lib/utmp.c: Replace UT_LINESIZE by a NITEMS() calculation

@alejandro-colomar alejandro-colomar marked this pull request as ready for review February 19, 2024 13:05
This was referenced Feb 19, 2024
Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good!

utmpx is specified by POSIX as an XSI extension.  That's more portable
than utmp, which is unavailable for example in musl libc.  The manual
page specifies that in Linux (but it probably means in glibc), utmp and
utmpx (and the functions that use them) are identical, so this commit
shouldn't affect glibc systems.

Assume utmpx is always present.

Also, if utmpx is present, POSIX guarantees that some members exist:

-  ut_user
-  ut_id
-  ut_line
-  ut_pid
-  ut_type
-  ut_tv

So, rely on them unconditionally.

Fixes: 170b76c ("Disable utmpx permanently")
Closes: <shadow-maint#945>
Reported-by: Firas Khalil Khana <firasuke@gmail.com>
Reported-by: "A. Wilfox" <https://github.com/awilfox>
Tested-by: Firas Khalil Khana <firasuke@gmail.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This silences a warning about an unused variable.

Tested-by: Firas Khalil Khana <firasuke@gmail.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This macro is useful to get the size of a member of a structure
without having a variable of that type.

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
A difference between 'struct utmp' and 'struct utmpx' is that
the former uses UT_LINESIZE for the size of its array members,
while the latter doesn't have a standard variable to get its
size.  Therefore, we need to get the number of elements in
the array with NITEMS().

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Collaborator Author

v4b changes:

$ git range-diff shadow/master gh/utmp utmp 
1:  1d755860 ! 1:  7e295795 lib/, src/, configure.ac: Use utmpx instead of utmp
    @@ Commit message
         Reported-by: Firas Khalil Khana <firasuke@gmail.com>
         Reported-by: "A. Wilfox" <https://github.com/awilfox>
         Tested-by: Firas Khalil Khana <firasuke@gmail.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## configure.ac ##
2:  05e9aeb7 ! 2:  661ce052 lib/utmp.c: get_session_host(): Reduce scope of variable
    @@ Commit message
         This silences a warning about an unused variable.
     
         Tested-by: Firas Khalil Khana <firasuke@gmail.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/utmp.c ##
3:  4628615a ! 3:  b962685e lib/sizeof.h: memberof(): Add macro
    @@ Commit message
         This macro is useful to get the size of a member of a structure
         without having a variable of that type.
     
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/sizeof.h ##
4:  64d45e98 ! 4:  7e72c19c lib/utmp.c: Replace UT_LINESIZE by a NITEMS() calculation
    @@ Commit message
         size.  Therefore, we need to get the number of elements in
         the array with NITEMS().
     
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/utmp.c ##

@alejandro-colomar alejandro-colomar merged commit 5ff6edf into shadow-maint:master Feb 20, 2024
9 checks passed
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 20, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <shadow-maint#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 20, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <shadow-maint#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 20, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <shadow-maint#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 21, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <shadow-maint#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this pull request Feb 21, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <shadow-maint#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit that referenced this pull request Feb 21, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit that referenced this pull request Feb 21, 2024
Recently, we started using utmpx instead of utmp, and we updated
<./configure.ac> to do the checks for 'struct utmpx' instead of
'struct utmp'.  However, I forgot to update the preprocessor
conditionals accordingly.

Fixes: 64bcb54 ("lib/, src/, configure.ac: Use utmpx instead of utmp")
Link: <#954>
Cc: Firas Khalil Khana <firasuke@gmail.com>
Cc: "A. Wilfox" <https://github.com/awilfox>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 1af6b68 ("lib/utmp.c: Use the appropriate autotools macros for struct utmpx")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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.

login and logoutd broken on systems with utmps
3 participants