GrayJack / coreutils

Core utils re-implementation for UNIX/UNIX-like systems written in Rust
Mozilla Public License 2.0
108 stars 40 forks source link

[BUG] `UtmpxSet::system()` shows killed sessions the same as active sessions #164

Closed starscouts closed 4 months ago

starscouts commented 4 months ago

Describe the bug When calling UtmpxSet::system(), the resulting set contains both active and killed (terminated abnormally, e.g. through a SIGKILL) sessions. There is no way to distinguish active and killed sessions.

To Reproduce Steps to reproduce the behavior:

  1. Terminate a session by killing (e.g. kill -9) all the processes associated with it.
  2. Notice how the TTY file in /dev disappears.
  3. Call UtmpxSet::system().
  4. Notice how the session still appears as UtmpxKind::UserProcess (or similar) instead of UtmpxKind::DeadProcess.

Expected behavior There is a way to distinguish the two session types, for instance through a new variant of UtmpxKind.

Desktop (please complete the following information):

Additional context This has caused us issues which we fixed in equestria-dev/where-rs#1, in which we worked around the issue by checking if the terminal associated with the session exists in /dev.

GrayJack commented 4 months ago

Hi! Thanks for this report,

I just revisited the code and the code is using the functions to get the raw utmpx data correctly according to POSIX and macOS documentations, and the conversion table from number to enum variant is still correct for macOS.

Could you try to dump the binary file format of utmpx before and after the kill and check if the file changed? I believe in macOS it is located at /var/run/utmpx file

If it is the same, it may be a system issue (?). If it indeed changed, there may be something platform-specific on macOS that I'm not aware or is undocumented

starscouts commented 4 months ago

Here are the 3 utmpx files (utmpx-before-session is before I create a session, utmpx-during-session is after I create a session using screen, and utmpx-after-session is after I kill it using the macOS system monitor, which I believe uses SIGKILL under the hood).

Do note that for all 3 of these, UtmpxSet::system() reports an active session on ttys001.

GrayJack commented 4 months ago

Sorry, I think you forgot to attach the files or send the link to them

starscouts commented 4 months ago

My bad, I forgot. Here they are, in a zip archive because GitHub wouldn't let me attach them otherwise. utmpx.zip

GrayJack commented 4 months ago

Thanks!

So, the before is different than the rest, as expected, but, the during and the after files are absolutely the same, that is the reason you are getting outdated information, the utmpx file are not being updated after you kill the session

starscouts commented 4 months ago

This is odd, I'm fairly certain the who command (more specifically who -a) shows the correct output.

GrayJack commented 4 months ago

I did a bit more research on that, looks like Darwin indeed does not set utmpx on kill, but who does an interesting thing, if it's a user process, it checks the stats of the ut_line to decide to print or not the row. Here is the link

starscouts commented 4 months ago

Interesting, this is pretty much what I have done in my own code (I used PathBuf::exists on the tty value). Do you think implementing this into your coreutils_core crate is within the scope or I should keep my workaround?

GrayJack commented 4 months ago

I think this would be within the scope, as long as it is as a method for Utmpx, as the Utmpx type is meant to be like the utmpx but higher level and with owned data

starscouts commented 4 months ago

I can make a pull request for this if needed

GrayJack commented 4 months ago

That sounds wonderful!

starscouts commented 4 months ago

Should I add a separate is_active method specific to macOS or do it another way?

GrayJack commented 4 months ago

I think you can add it for all platforms. Looks like all BSD uses this logic as well, not sure about Linux, but having this logic as a fallback is not a bad thing