Correct finding UTMP entry

[Karlson2k]

This PR fixes wrong utmp entry search, by better following algorithm expected by Linux kernel as described here: https://man7.org/linux/man-pages/man5/utmp.5.html#DESCRIPTION

[Karlson2k]

PR has been updated to perform a minimal changes. Extra refactoring was excluded.

[Karlson2k]

@alejandro-colomar Rewritten again.

[Karlson2k]

Updated as requested (not counting extra getpid() call which is unsafe in terms of being future-proof).

[Karlson2k]

Updated PR to fix only utmp entry search (align with Linux kernel description).

[Karlson2k]

@alejandro-colomar Updated as requested, except this comment, for which the code is changed differently.

[alejandro-colomar]

Updated utmp entry search algorithm to follow GNU/Linux description:
https://man7.org/linux/man-pages/man5/utmp.5.html#DESCRIPTION

An entry is found by looking for matching PID.  If several such entries
found (for example, due to cleanup failure of old entries) then first
entry with both matching PID and matching 'ut_line' (current terminal)
is used.  If not entry has matching 'ut_line' then first entry with
matching PID is used (if getty/init process does not set 'ut_line').

When no single entry is matched by PID, then but at least one entry is
matched current terminal the the first such entry is selected (if getty
does not set correct PID).

Could you quote specific parts of utmp(5) to defend the correctness of your claims?

Why is it valid to use an entry with matching PID but not matching ut_line?

Why is it valid to use an entry matching the current terminal but not the PID?

[Karlson2k]

Here is (a shortened and a bit simplified) description of the process from https://man7.org/linux/man-pages/man5/utmp.5.html#DESCRIPTION:

The first entries ever created result from init(1) processing inittab(5). If no empty record with the needed ut_id can be found, init(1) creates a new one. It sets ut_id from the inittab, ut_pid and ut_time to the current values, and ut_type to INIT_PROCESS.

getty locates the entry by the PID, changes ut_type to LOGIN_PROCESS, changes ut_time, sets ut_line, and waits for connection to be established. login(1), after a user has been authenticated, changes ut_type to USER_PROCESS, changes ut_time, and sets ut_host and ut_addr. Depending on getty and login(1), records may be located by ut_line instead of the preferable ut_pid.

Old version used entry only with both matches ut_pid and ut_line. This was not expected, match by PID is enough.
However, for safety reasons, the new code tries to match both PID and line. (In case if cleanup was not performed correctly).
If perfect match (like the old one) was not found, then the first match by PID is used (this is what described in the documentation).
If no entry matched PID, then used first entry which matched by ut_line. (Assuming that no two sessions are possible simultaneously on the same console.) This is also described in the documentation: records may be located by ut_line instead of the preferable ut_pid.

However, if user manually started login binary after being already logged in the console, previous utmp entry matched by ut_line will not be used as it has USER_PROCESS type, which is ignored when matching only by ut_line (entry must be precisely LOGIN_PROCESS). This situation is not described in specification. login will create a new entry in such case (the same was before this patch).

[Karlson2k]

Let me know if any further changes are needed.

[alejandro-colomar]

Let me know if any further changes are needed.

I'm okay with the source code.

However, I'd feel more comfortable if @hallyn and/or @ikerexxe could review the logic.

[ikerexxe]

I've been trying to review this patch all week, but every time I started, something else caught my attention. Now I've had a good look at it, and I would say that the change in prepare_utmp() is incorrect. See the inline comment for more details.

[Karlson2k]

I've been trying to review this patch all week, but every time I started, something else caught my attention. Now I've had a good look at it, and I would say that the change in prepare_utmp() is incorrect. See the inline comment for more details.

Nice catch, thanks!

Corrected.

[ikerexxe]

LGTM. Good job!

[Karlson2k]

@ikerexxe Thank you!