Allow passing inheriting PTY from caller

[chergert]

Feature request description

First off, just to help give context, think of gnome-terminal on Fedora where creating a new tab inherits the container of the last tab. Each tab already has it's own PTY created on demand and the terminal tab uses that PTY to answer questions about things like "what is the foreground process".

It would be very nice if I had more options than podman exec --tty where a new PTY is allocated. I know the "security implications" of sharing an existing PTY, but in many cases you don't want/need to deal with that. The loss of having real functional TTY functions like tcgetpgrp() is just too much due to the intermediary PTY.

Suggest potential solution

We already have --tty where a new TTY is allocated. If omitted we already get pipes instead. What would be nice is a middle ground where we can just inherit the current TTY.

Have you considered any alternatives?

I go through dark arts to peek on containers through PTY ownership and I'd rather not when tcgetpgrp() is right there.

Additional context

No response

[giuseppe]

so you'd like the equivalent of podman run --log-driver passthrough ... for a TTY?

We have some checks in place, both in Podman and conmon, to disallow it for security reasons; but we can drop these or add a new mode (passthrough-tty) if it turns out to be useful.

I've done a quick test dropping these checks, but that doesn't seem enough:

$ bin/podman --conmon ~/src/conmon/bin/conmon  run --ipc=host --pid=host --log-driver passthrough --rm fedora  bash
bash: cannot set terminal process group (169877): Inappropriate ioctl for device
bash: no job control in this shell
[root@90fbc4aa2da8 /]# ls -l /proc/self/fd
total 0
lrwx------. 1 root root 64 Nov 23 23:04 0 -> /dev/pts/0
lrwx------. 1 root root 64 Nov 23 23:04 1 -> /dev/pts/0
lrwx------. 1 root root 64 Nov 23 23:04 2 -> /dev/pts/0
lr-x------. 1 root root 64 Nov 23 23:04 3 -> /proc/170062/fd

[chergert]

Thanks for looking into this so quickly!

so you'd like the equivalent of podman run --log-driver passthrough ... for a TTY?

I think so? I'm not really familiar with the other parts of podman though.

I've done a quick test dropping these checks, but that doesn't seem enough:

This is probably just because your TTY already has a controller. It's not really testable in this fashion unless you create a new PTY from the outside with O_NOCTTY and have that bound as stdin/out/err (then do the ioctl() to set it as controller after fork()). This would model the behavior you'd see when doing something like a new VtePty for a VteTerminal in GTK.

[chergert]

You might try doing something like screen -dmS test-podman bin/podman ... then screen -r test-podman to see if that works since I think you'd get a new PTY which your podman can pass through to eventually bash which will become the controller of the PTY.

[Luap99]

I guess this is basically the same issue as #18001?

[chergert]

When I originally read that issue, it seemed like what they needed might be slightly different. But I think if there was finite control for each of 0..2 then this could implemented using that.

[giuseppe]

screen -dmS

yes, thanks. That works.

tcgetpgrp(0) still fails with ENOTTY from the container:

247757 ioctl(0, TIOCGPGRP, 0x7ffc1c75c334) = -1 ENOTTY (Inappropriate ioctl for device)

Do you expect it to work from the container?

IMO, inheriting the TTY could be achieved either by relaxing passthrough to accept a TTY, or having a more explicit mode passthrough-tty so the user is aware of the risks.

[chergert]

Do you expect it to work from the container?

tcgetpgrp() must be called on the PTY master and would be expected to fail if called on the PTY side mapped into the child process.

[giuseppe]

I've opened a PR: #20774

I still need to figure out a way to test the feature, locally I've used a Python script:

import subprocess
import sys
import os
import time

command = ["bin/podman", "--conmon", "/usr/local/bin/conmon", "run", "-d", "-v/dev:/dev", "--log-driver", "passthrough-tty", "--rm", "fedora", "sh"]

(master, slave) = os.openpty()

process = subprocess.Popen(command, stdin=slave, stdout=slave, stderr=slave)

process.wait()

print("tcgetpgrp: %d\n" % os.tcgetpgrp(master))

Please let me know if this does what you were asking for

[chergert]

Please let me know if this does what you were asking for

If os.tcgetpgrp() is giving you the PID of sh then yeah that's just perfect.

[halfline]

Do you expect it to work from the container?

tcgetpgrp() must be called on the PTY master and would be expected to fail if called on the PTY side mapped into the child process.

I don't think that's generally true. i mean tcgetpgrp() is a general use function for all terminals, not just /dev/pts/N but /dev/ttyN, /dev/ttySN etc... (so it would be weird if it only worked on the half of the ptm/pty pair that the other terminal types don't have any equivalent of)

I think it's likely failing in this case because the container has a different /dev/pts filesystem mounted than the host, but that's just a guess.

Does job control in bash work okay in this scenario? i mean jobs/fg/bg etc ?

[chergert]

I think it's likely failing in this case because the container has a different /dev/pts filesystem mounted than the host, but that's just a guess.

Yes, if you check out the kernel code, you'll find the sending the pid to the syscall gets converted to 0 because of this. Fun fact, 0 is the only value not documented to be possible to be returned.

Does job control in bash work okay in this scenario? i mean jobs/fg/bg etc?

Yeah it works fine. The issue is that we have the PTY from VTE, with the child side mapped to 0/1/2 of the child process. That child process (podman) then spawns the desired process (bash in this case) but with a PTY it has itself created and I believe is proxying. So if you tcgetpgrp() on the VTE provided PTY, it will be the pid of the podman process, not the actual foreground process that is a child of podman.

This is problematic because we would want to close a terminal tab without asking the user if they want to kill the process in the case the real foreground process is bash. The intermediary podman process is not interesting to us.

Since there is an extra PTY there, it's difficult to determine what is the real foreground of that sub PTY without looking at /proc and making outlandish guesses.

[halfline]

I think it's likely failing in this case because the container has a different /dev/pts filesystem mounted than the host, but that's just a guess.

Yes, if you check out the kernel code, you'll find the sending the pid to the syscall gets converted to 0 because of this. Fun fact, 0 is the only value not documented to be possible to be returned.

heh, seems like there could be improvement here... i mean the host /dev/pts should be mounted in /run/host so it's not like the device node is completely absent....

Does job control in bash work okay in this scenario? i mean jobs/fg/bg etc?

Yeah it works fine.

ah okay good.

The issue is that we have the PTY from VTE, with the child side mapped to 0/1/2 of the child process. That child process (podman) then spawns the desired process (bash in this case) but with a PTY it has itself created and I believe is proxying.

Actually i think its conmon and runc not podman directly, but that's neither here nor there...

So if you tcgetpgrp() on the VTE provided PTY, it will be the pid of the podman process, not the actual foreground process that is a child of podman.

right, but i guess my point is tcgetpgrp is a useful call for programs running in the container too, and with this change those programs are going to start getting errors when they call it. maybe it doesn't matter much in practice.

This is problematic because we would want to close a terminal tab without asking the user if they want to kill the process in the case the real foreground process is bash. The intermediary podman process is not interesting to us.

Since there is an extra PTY there, it's difficult to determine what is the real foreground of that sub PTY without looking at /proc and making outlandish guesses.

I wonder why the container doesn't share the same pts filesystem as the host. at least for toolbox type containers it seems like there's little reason for it be sandboxed

[chergert]

I wonder why the container doesn't share the same pts filesystem as the host. at least for toolbox type containers it seems like there's little reason for it be sandboxed

That sounds useful too.

[halfline]

actually i wonder if things would "just work" with this change and if vte set up 0/1/2 for its child to an fd that opened g_build_filename ("/run/host/", ttyname(granted_pty_fd), NULL); instead using the fd from posix_openpt (or whatever) directly that prefixed the pty name with /run/host

EDIT: Rereading this, this morning I realize my pseudocode was confusing because I got details of the api wrong...updated for clarity.

[chergert]

In most cases it's just opening the path from ptsname_r() right?

https://elixir.bootlin.com/glibc/glibc-2.38/source/sysdeps/posix/ttyname.c#L77 Is this really implemented by checking device/inode/etc and looping through the directory? If so, I don't think it would work because they inodes won't match in /dev/.

[halfline]

You're totally right, in most cases the terminal is going to be using ptsname already anyway, so it could just prepend /run/host to that. The details on how to get the pty device name on the host aren't super important to what i'm saying though...which of ptsname on the ptm or ttyname / ptsname / minor(stat_buf.st_dev) on the pty... whatever is used isn't of much consequence. I'm sure e.g. VTE has the ability to get the the pty device name on the host in some way and that way will work. I'm saying on the host the root file system is mounted at / AND at /run/host. in the container / is different from / on the host. but in the container /run/host is the same as /run/host on the host. If on the host the pty is opened via /run/host/dev/pts/N instead of /dev/pts/N then it at least seems plausible to me that tcgetpgrp will work in the container. but it's just a guess.

I don't think you're looking at the right implementation of ttyname since it doesn't even seem to have pty support. maybe that's for glibc on non-linux platforms?

The inodes will match between e.g. /dev/pts/0 and /run/host/dev/pts/0 on the host, and won't match between /dev/pts/0 and /run/host/dev/pts/0 in the container. Though I suspect it's more about the associated ptmx device than the inodes of the devices themselves (though same story either way). So, depending on how ttyname is implemented I guess it's possible that using /run/host/dev/pts/0 could leave ttyname in the guest dysfunctional.

EDIT: Also updated this comment for clarity

[halfline]

ah i think ttyname is implemented here:

https://elixir.bootlin.com/glibc/glibc-2.38/source/sysdeps/unix/sysv/linux/ttyname_r.c

It looks like it just calls readlink("/proc/self/fd/%d", fd) so i think it should work in the guest.

[halfline]

actually looking at vte right now, it uses TIOCGPTPEER not ptsname so probably do need use ttyname indeed.

[halfline]

So just to be clear, this is what i'm suggesting along with @giuseppe 's change in podman.
(note, only compile tested, I didn't see if it works or anything).

--- a/src/pty.cc
+++ b/src/pty.cc
@@ -40,2 +40,3 @@
 #include <sys/socket.h>
+#include <sys/stat.h>
 #include <errno.h>
@@ -96,17 +97,4 @@ Pty::unref() noexcept
 int
-Pty::get_peer(bool cloexec) const noexcept
+Pty::get_peer_fd(int fd_flags) const noexcept
 {
-        if (!m_pty_fd)
-                return -1;
-
-        /* FIXME? else if (m_flags & VTE_PTY_NO_CTTTY)
-         * No session and no controlling TTY wanted, do we need to lose our controlling TTY,
-         * perhaps by open("/dev/tty") + ioctl(TIOCNOTTY) ?
-         */
-
-        /* Now open the PTY peer. Note that this also makes the PTY our controlling TTY. */
-        auto const fd_flags = int{O_RDWR |
-                                  ((m_flags & VTE_PTY_NO_CTTY) ? O_NOCTTY : 0) |
-                                  (cloexec ? O_CLOEXEC : 0)};
-
         auto peer_fd = vte::libc::FD{};
@@ -154,4 +142,39 @@ Pty::get_peer(bool cloexec) const noexcept
 
+        return peer_fd.release();
+}
+
+int
+Pty::get_peer(bool cloexec) const noexcept
+{
+        if (!m_pty_fd)
+                return -1;
+
+        /* FIXME? else if (m_flags & VTE_PTY_NO_CTTTY)
+         * No session and no controlling TTY wanted, do we need to lose our controlling TTY,
+         * perhaps by open("/dev/tty") + ioctl(TIOCNOTTY) ?
+         */
+
+        /* Now open the PTY peer. Note that this also makes the PTY our controlling TTY. */
+        auto const fd_flags = int{O_RDWR |
+                                  ((m_flags & VTE_PTY_NO_CTTY) ? O_NOCTTY : 0) |
+                                  (cloexec ? O_CLOEXEC : 0)};
+
+        auto peer_fd = vte::libc::FD{};
+
+        peer_fd = get_peer_fd(fd_flags);
+
         assert(bool(peer_fd));
 
+        if (is_exportable_to_containers()) {
+                const char *tty_name = ttyname(peer_fd.get());
+                std::string global_peer = "/run/host" + std::string(tty_name);
+                auto global_peer_fd = vte::libc::FD{};
+
+                global_peer_fd = ::open(global_peer.c_str(), fd_flags);
+
+                if (global_peer_fd != -1) {
+                        peer_fd.swap(global_peer_fd);
+                }
+        }
+
 #if defined(__sun) && defined(HAVE_STROPTS_H)
@@ -180,2 +203,27 @@ Pty::get_peer(bool cloexec) const noexcept
 
+bool
+Pty::is_exportable_to_containers() const noexcept
+{
+#ifdef __linux__
+        struct stat m_pty_stat, global_ptmx_stat;
+
+        if (::fstat(m_pty_fd.get(), &m_pty_stat) == -1) {
+            auto errsv = vte::libc::ErrnoSaver{};
+                _vte_debug_print(VTE_DEBUG_PTY, "%s failed: %s\n", "fstat", g_strerror(errsv));
+                return false;
+        }
+
+        // Check if /run/host/dev/pts/ptmx is an alias to the current pty master
+        // If so, we should use it instead so containers (with their own /dev/pts)
+        // can fully use the pty
+        if (::stat("/run/host/dev/pts/ptmx", &global_ptmx_stat) != -1 &&
+            m_pty_stat.st_dev == global_ptmx_stat.st_dev &&
+            m_pty_stat.st_ino == global_ptmx_stat.st_ino) {
+                return true;
+        }
+#endif
+
+        return false;
+}
+
 void
diff --git a/src/pty.hh b/src/pty.hh
index 2dc8ac97..efa31067 100644
--- a/src/pty.hh
+++ b/src/pty.hh
@@ -35,2 +35,4 @@ private:
 
+        bool is_exportable_to_containers() const noexcept;
+        int get_peer_fd(int fd_flags) const noexcept;
 public:

[chergert]

@halfline I don't use VTE spawn API in any of my projects (it's all done manually so I have more control), so I can't test this patch. But I can make something that does the same thing quick in Prompt/GNOME Builder/etc.

[halfline]

ah okay i didn't realize that. cool, the main point of the patch was just to clarify what i was trying to say anyway since i felt like i was failing to articulate it well.

[chergert]

It doesn't look like this will work. I get different st_dev and st_ino for both. For example I get for the PTY master something like 5 and 26. And for the /run/host/dev/pts/ptmx something like 88 and 2.

[chergert]

I don't want to spam this thread any further, so @halfline and I can take our VTE issues to the GNOME tracker somewhere. But one last thing, if you feel like testing, https://gitlab.gnome.org/chergert/prompt/-/commit/a97f790ed13b22eaf8f0f9ecd3aaae45cd86b65d adds a commit to Prompt which you can test this via Flatpak.

Once we have a podman that is updated on the host, we can tweak our command line arguments to include passthrough-tty.

[halfline]

It doesn't look like this will work. I get different st_dev and st_ino for both. For example I get for the PTY master something like 5 and 26. And for the /run/host/dev/pts/ptmx something like 88 and 2.

ahhh, I guess that makes sense though it's inconvenient. Every ptm has a different end point so it's really a different device than the multiplexer.

Well, we just have to change the the check a little... either fstat the pts instead of the ptm side or compare /dev/pts/ptmx to /run/host/dev/pts/ptmx directly instead fstat on theptm fd.

[halfline]

okay i created https://gitlab.gnome.org/chergert/prompt/-/merge_requests/4 for us to have a place to move the discussion and experiments

[chergert]

Is there anything I can do to help this along?

I have a number of features in Prompt that I'll be able to fix for Podman/Toolbox containers once it's in place.

[giuseppe]

we are just waiting for a new conmon release to be included in the CI images and then we can merge #20774

[chergert]

Thanks so much for taking the time to reply! Sounds great!