Extensions: UseSystemd can detect an interactive shell as `running as systemd service`

[Tragetaschen]

Describe the bug

I have my embedded platform and configure the IHostBuilder to .UseSystemd().

1) Using the application as systemd service always works correctly.
2) When I log into my system using SSH, I can run my application on the interactive prompt and it's using ConsoleLifetime and console logging format.
3) When I use the tty on the serial port to run the application, that's detected as systemd environment and I get the systemd log format and termination behavior (no SIGINT handler)

The reason for the latter case is that my bash instance on the serial port inherits the INVOCATION_ID environment variable through login:

# env
LANG=C.UTF-8
OLDPWD=/
INVOCATION_ID=5561c5304f434119a45c4b6f260f1b00
EDITOR=vi
XDG_SESSION_ID=c1
HUSHLOGIN=FALSE
USER=root
PWD=/root
HOME=/root
JOURNAL_STREAM=8:13520
XDG_SESSION_TYPE=tty
MAIL=/var/spool/mail/root
SHELL=/bin/sh
TERM=putty
XDG_SESSION_CLASS=user
SHLVL=1
LOGNAME=root
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/0/bus
XDG_RUNTIME_DIR=/run/user/0
PATH=/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
PS1=\u@\h:\w\$
_=/usr/bin/env
# pstree
systemd-+-…
        |-login---sh---pstree
…

[anurse]

@tmds do you have thoughts here? You're our semi-resident systemd expert ;). We can take a look as well but you might have some insight here.

[anurse]

Looking in to this deeper, it looks like a tricky situation. Right now we're using the INVOCATION_ID as the primary detector. If it's present, we don't look deeper, but if the shell is started from systemd, the env vars will flow from process to process.

We do have our fallback detection logic that looks at /proc/[pid]/comm, but some sources show that to be risky at times.

One option is, of course, to perform your own validation and use that to determine whether to call UseSystemd.

[tmds]

@anurse I had missed your notification.
I will take a closer look.

Strange enough, INVOCATION_ID is even set when I run an interactive terminal on my Fedora 31. I'm pretty sure this wasn't always like this.

[tmds]

To fix this, we could check if the process itself or any of its parents is a terminal. If that is the case, we're not a systemd service.
Something like this:

        private static bool HasTtyInProcessTree()
        {
            if (!Console.IsErrorRedirected)
            {
                return true;
            }
            else
            {
                int myPid = Process.GetCurrentProcess().Id;
                int? ppid = GetPPid(myPid);
                if (ppid.HasValue)
                {
                    return HasTtyInProcessTree(ppid.Value);
                }
            }

            return false;
        }

        private static bool HasTtyInProcessTree(int pid)
        {
            if (pid == 0)
            {
                return false;
            }

            if (IsATty($"/proc/{pid}/fd/2"))
            {
                return true;
            }
            else
            {
                int? ppid = GetPPid(pid);
                if (ppid.HasValue)
                {
                    return HasTtyInProcessTree(ppid.Value);
                }
            }

            return false;
        }

        private static int? GetPPid(int pid)
        {
            try
            {
                string statContent = File.ReadAllText($"/proc/{pid}/stat");
                int pidStart = statContent.IndexOf(") ") + 4;
                int pidEnd = statContent.IndexOf(" ", pidStart);
                System.Console.WriteLine($"'{statContent.Substring(pidStart, pidEnd - pidStart)}'");
                return int.Parse(statContent.Substring(pidStart, pidEnd - pidStart), CultureInfo.InvariantCulture);
            }
            catch
            {
                return null;
            }
        }

        private static bool IsATty(int fd)
        {
            try
            {
                return isatty(fd) == 1;
            }
            catch
            {
                return false;
            }
        }

        private static bool IsATty(string filename)
        {
            try
            {
                using (var filestream = new FileStream(filename, FileMode.Open, FileAccess.Read))
                {
                    return IsATty(filestream.SafeFileHandle.DangerousGetHandle().ToInt32());
                }
            }
            catch
            {
                return false;
            }
        }

        [DllImport("libc", SetLastError = true)]
        private static extern int isatty(int fd);

@anurse wdyt?
@onyxmaster do you have some thoughts on the issue, or suggested solution?

[onyxmaster]

I wonder if redirected input/output should be considered as the "terminal" case.

Maybe drop the INVOCATION_ID check altogether and always go through the "slow" path with detecting the name of the parent process? Yes, I see that parent pid might actually be not named with systemd, but it looks less complex.

Also, checking the TERM envvar might be useful, if it's not empty, it usually means that there is _some_ terminal is attached to the session.

[Tragetaschen]

Would INVOCATION_ID != null && GetPPid() == 1 work as detector for systemd?

[onyxmaster]

@Tragetaschen no, since it would not handle the user units. Also, if systemd is not an init process it would be a false positive. Take a look at the current implementation, we could skip the current short-circuit with INVOCATION_ID presence check, but I'm still not sure if it is the right way (see my previous comment).

[Tragetaschen]

Ah, forgot about user units

[tmds]

Maybe drop the INVOCATION_ID check altogether and always go through the "slow" path with detecting the name of the parent process

I'm not concerned with the speed. This does require the systemd process to be the direct parent. Maybe that is ok.

[tmds]

checking the TERM envvar might be useful

And a much simpler check than what I had in mind.

[anurse]

I'm not concerned with the speed.

I concur. This is a one-time check at app startup. "Slow" is very relative here. Perhaps a rough measurement would be useful, but I'm not worried. The parent process check seems right to me.

I think we should also add some kind of override. For example UseSystemd(bool force). If force is true, the check is skipped and we assume the service is a Systemd service. That way we could be a little more conservative and users could always force it on in other places. Users already have a workaround for adding custom "disable" logic (add their logic and skip the call to UseSystemd if they don't believe they're in systemd). Adding that would give a way to have custom "enable" logic as well.

[anurse]

@tmds Thoughts on the action we take here? To me, checking the parent process name (and providing a force override) seems right. I can find someone to do the work, but I'd appreciate your thoughts as someone with a little more systemd experience 😄.

[onyxmaster]

The TERM envvar is a best indicator of the terminal being present. Oh the other hand, the method checks for process being run is a systemd service.
Maybe UseSystemd should check for parent file name (ignoring INVOCATION_ID altogether), but I don't think checking for systemd presence and terminal availability should influence each other.
I don't see for example why a systemd service cannot (in theory) have a terminal attached to it. I agree that it should use different logging format, yes, but it would not stop being a systemd service.

So, maybe adjust "UseSystemd" to skip the INVOCATION_ID "shortcut" and always go the full path, along with adding some kind of a similar check SystemdHelpers.HasTerminalAttached and adding checking it it around here?

P.S. I'm not Tom, but since at least part of the code in question was written by me I feel an obligation at least to provide relevant (to the best of my knowledge) input on the topic.

[tmds]

Maybe UseSystemd should check for parent file name (ignoring INVOCATION_ID altogether)

Yes, that is what we'll do.

The more liberal check would be: INVOCATION_ID set, TERM not set.

and providing a force override)

Maybe make this a try-state? force on, force off, 'default'

can find someone to do the work,

I can look into it next week also.

[onyxmaster]

I'd like to stress the fact that being a systemd service and being attached to terminal are not mutually exclusive. I think treating them as a same "flag" could be wrong for certain kinds of applications.

But yes, adding the ability to override the logic behind both of these flags would be very helpful in non-standard scenarios.

[anurse]

Maybe make this a try-state? force on, force off, 'default'

We already have force off and 'default' today. To force off, just check before calling UseSystemd. @Tratcher pointed out that we also have a form of force on today by just putting the content of UseSystemd in your own ConfigureServices (everything is public):

services.Configure<ConsoleLoggerOptions>(options =>
{
    options.Format = ConsoleLoggerFormat.Systemd;
});

services.AddSingleton<ISystemdNotifier, SystemdNotifier>();

services.AddSingleton<IHostLifetime, SystemdLifetime>();

UseSystemd(SystemdMode.Off) seems weird to me, even if it's UseSystemd(ShouldEnableSystemd()). Having a parameter that means "make this method no-op" feels... off. Also, the Windows Service equivalent does not have a force option either and does a similar kind of parent process check.

Given the existing workaround and the fact that in general we're going to try and do the right thing. I could probably go without the force parameter entirely.

I can look into it next week also.

Cool, just let me know if you need some help!

[davidfowl]

This feels patch worthy...

[anurse]

This feels patch worthy...

There's a clear workaround here though, which is to have your own conditional logic prior to calling UseSystemd. Generally that means it wouldn't meet the bar (especially given that it does have a breaking change angle).

[davidfowl]

@anurse should we doc this then?

[anurse]

We don't have a Systemd doc that I can find.