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

alejandro-colomar commented 5 months ago

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: 170b76cdd1a9 ("Disable utmpx permanently") Closes: https://github.com/shadow-maint/shadow/issues/945 Reported-by: @firasuke Reported-by: @awilfox

alejandro-colomar commented 5 months ago

I suggest reviewing with

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

alejandro-colomar commented 5 months ago

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 commented 5 months ago

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

alejandro-colomar commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

And I can confirm it is working as intended!

alejandro-colomar commented 5 months ago

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 commented 5 months ago

v4b changes:

Reviewed-by: @ikerexxe

$ 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 ##

shadow-maint / shadow

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