Fix Unix98 pty allocation under a sysroot - #266
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
5d08fa9 to
5df4e18
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8b743df to
b2859de
Compare
glibc's posix_openpt() opens /dev/ptmx and then confirms devpts is mounted before handing the master back. statfs did not identify /dev/pts, so glibc closed a working master and every glibc pty consumer failed -- terminal emulators, script(1), expect, tmux, screen, sshd, anything using openpty() or forkpty(). Report devpts from statfs and fstatfs, following the per-filesystem pattern already used for proc and tmpfs. That exposed two more links in the same chain. grantpt(3) chmods the slave when it thinks the mode is wrong, and /dev/pts/N is virtualized for open and stat but not for chmod, so the call fell through to the host and failed with ENOENT. glibc then fell back to the pt_chown helper, which no modern distro ships. Intercept chmod and chown on a live slave instead, where the mode and owner come from the pty layer. The synthesized slave stat also reported the host file owner rather than the guest uid, which is why grantpt wanted to chown at all. Report the opening uid and the tty group, matching what devpts gives. tests/test-devpts.c covers the statfs magic, posix_openpt, fstatfs agreement, the grantpt sequence, a master/slave round trip and a negative control that an ordinary path is not devpts. The round trip runs in raw mode under an alarm so a stuck pty fails the test rather than hanging the run. Fix sysprog21#263
b2859de to
eb70a6c
Compare
jserv
left a comment
jserv
left a comment
There was a problem hiding this comment.
The fix is real and I verified it: built both branches with the aarch64-linux-gnu toolchain (glibc 2.28, which does have the statfs check in getpt.c and the old chmod/chown grantpt). tests/test-devpts fails at steps 1 and 2 on main and passes all 7 steps on this branch. Locking is right, pty_lookup_slave_path takes pty_keepalive_lock. tx.intercept_path is the correct selector under --sysroot, host_path would be wrong. The new test is picked up by the existing mk/config.mk wildcard.
What follows is from a probe run under this branch:
statfs(/dev/pts/016) -> 0x1cd1 statfs(/dev/pts/+16) -> 0x1cd1
statfs(/dev/pts/ 16) -> 0x1cd1 open(/dev/pts/016) -> 4 OPENED
chmod(slave,0600)=0, stat after -> mode=0620
fstatfs(master) f_type=0x13 fstatfs(slave) f_type=0x13
fstatat(ptsdir,"16") mode=0444 rdev=0x0 NOT a chardev
Two notes I am not asking you to fix here. Relative access through a /dev/pts dirfd resolves to a 0444 scratch stub rather than the pty (last probe line), but that is pre-existing on main and not a regression from this PR. And the matrix runs unit tests under musl, where grantpt is a no-op, so the glibc path this PR exists for is not exercised in CI.
|
|
||
| int64_t sys_fstatfs(guest_t *g, int fd, uint64_t buf_gva) | ||
| { | ||
| /* Deliberately no devpts case for a pty master fd: Linux answers from |
There was a problem hiding this comment.
Issue #263 reports two bugs and this fixes one of them. f_type is still passed straight through from the macOS struct statfs, so the guest sees macOS mount-type indices rather than Linux superblock magics. Measured on this branch:
fstatfs(master) f_type=0x13
fstatfs(slave) f_type=0x13
statfs("/") f_type=0x1a
None of those are Linux magics. That is the second half of what #263 describes (lin->f_type = mac->f_type). Worth saying in the PR body whether it is deliberately out of scope, because as it stands the issue is only half closed and the untranslated value is now load-bearing for test 7 in the new test file.
| } | ||
| printf("PASS (fd=%d)\n", master); | ||
|
|
||
| /* No assertion on fstatfs(master): Linux answers from whatever filesystem |
There was a problem hiding this comment.
This rationale is about the master fd, and it is sound there. It does not cover the slave fd, which is unambiguous: on Linux fstatfs on an open /dev/pts/N reports devpts, because the slave really does live on that mount. Measured here it reports 0x13, the macOS type index.
That gap has a real caller. glibc up to 2.32 has grantpt short-circuit on a devpts fstatfs, and other code sniffs the fs of a tty the same way. Either handle the slave fd in sys_fstatfs (the fd already carries the pty identity) or extend this comment to say the slave case is knowingly left alone.
| * unallocated or malformed /dev/pts/N, so only a live slave may claim | ||
| * devpts here -- everything else falls through and misses on the host. | ||
| */ | ||
| if (!strcmp(path, "/dev/ptmx") || !strcmp(path, "/dev/pts") || |
There was a problem hiding this comment.
Claiming devpts for /dev/ptmx here contradicts the reasoning this same PR gives at sys_fstatfs: that the filesystem behind /dev/ptmx is devpts only when it resolves to /dev/pts/ptmx, and devtmpfs or tmpfs otherwise. If there is no single correct value for the fd, there is none for the pathname either.
It is also not needed. glibc's getpt.c statfs's /dev/pts and /dev, never /dev/ptmx. Suggest dropping /dev/ptmx from this predicate, or, if something does need it, say what and drop the fstatfs rationale instead so the two agree.
| if (!*digits) | ||
| return false; | ||
| char *endp; | ||
| unsigned long n = strtoul(digits, &endp, 10); |
There was a problem hiding this comment.
strtoul is too permissive for a devpts dentry name. It skips leading whitespace, accepts a + sign, and accepts leading zeros, so with slave 16 live:
statfs("/dev/pts/016") -> 0x1cd1
statfs("/dev/pts/+16") -> 0x1cd1
statfs("/dev/pts/ 16") -> 0x1cd1
open("/dev/pts/016") -> 4 (a working slave fd)
Linux answers ENOENT for all three. The open and stat laxness is pre-existing, this PR only moved it, but it now also decides devpts identity and whether chmod and chown are intercepted, so it is worth tightening while the parser is being extracted anyway. Requiring ASCII digits only, with no leading zero unless the whole name is "0", is a few lines and removes the aliasing.
A case for test 5 would fall out of it: /dev/pts/0<live N> currently resolves where it should not.
| return 0; | ||
| } | ||
|
|
||
| bool proc_path_is_pty_slave(const char *path) |
There was a problem hiding this comment.
This duplicates machinery that is already here. path_might_use_stat_intercept() in src/syscall/path.c:91 already matches /dev/pts, /dev/pts/ and /dev/pts/N, and proc_intercept_stat() already returns 0 only for a live slave (it does the same pty_lookup_slave_path call). sys_faccessat uses exactly that pair for this purpose already.
So the three new call sites can be path_might_use_stat_intercept(p) && proc_intercept_stat(p, &st) == 0, and this export plus the header declaration can go. sys_fchownat in particular calls this helper and then proc_intercept_stat right after, so it walks the locked pty table twice and opens a window between the two where the slave can go away.
| * a Linux pty slave away either, so refuse rather than report a success | ||
| * the next stat() contradicts. | ||
| */ | ||
| if (proc_path_is_pty_slave(tx.intercept_path)) { |
There was a problem hiding this comment.
This branch has no known caller once the stat fix in this same PR lands. glibc's grantpt chowns only when st.st_uid != __getuid(), and reporting proc_get_uid() from the synthesized stat makes that false by construction. musl's grantpt is a no-op. Verified on this branch: grantpt returns 0 and never chowns.
What it does buy is a divergence. A guest running as root gets EPERM where Linux would let the chown through, which is the login and sshd case for handing a tty to a user. Suggest deleting the branch and letting the pass-through fail as it does today, or, if it stays, saying in the comment that it exists to keep a future non-matching uid honest rather than to serve grantpt.
| * difference; keeping it would need per-slave state that also has to cross | ||
| * the fork-IPC boundary. | ||
| */ | ||
| if (proc_path_is_pty_slave(tx.intercept_path)) |
There was a problem hiding this comment.
This is the branch that actually carries the fix, and nothing tests it. Test 6 covers chown only, and under musl (what the matrix runs) grantpt is a no-op, so neither is reached there.
It is reached on glibc, and reliably so. grantpt computes the mode it wants from getgrnam("tty"). On a macOS host that group is gid 4, while PTY_SLAVE_TTY_GID here is 5, so st.st_gid == gid is false, the wanted mode comes out 0600 against the synthesized 0620, and grantpt chmods every time. Remove this branch and grantpt fails.
Two asks. Add a chmod case to tests/test-devpts.c so the branch cannot be deleted silently. And consider whether the comment should mention that the gid mismatch against the host group database is what makes the chmod unconditional, since that is not obvious and it is the reason 0620 is never accepted as-is.
|
|
||
| printf "\nSyscall coverage\n" | ||
| test_check "$runner" "test-file-ops" "0 failed" "$bindir/test-file-ops" | ||
| test_check "$runner" "test-devpts" "PASS|0 failed" "$bindir/test-devpts" |
There was a problem hiding this comment.
PASS matches every per-step line the test prints, not just the summary, so this pattern is satisfied by step 1 alone. The rc check in evaluate_result is what actually catches a failure today, which makes the pattern decoration rather than a gate.
Anchoring it on the final line is the same amount of typing: "all tests passed -- PASS". Every neighbouring entry uses "0 failed" for the same reason, so this would also stop being the one line in the block with a two-alternative pattern.
| * open). Such a path has no host backing, so metadata operations on it must be | ||
| * answered here rather than passed through to the host filesystem. | ||
| */ | ||
| bool proc_path_is_pty_slave(const char *path); |
There was a problem hiding this comment.
This is the first bool in the header, and the header does not include <stdbool.h>. It compiles today only because core/guest.h happens to pull it in. Add the include, or drop the declaration entirely per the duplication note on procemu.c.
2 participants
glibc's posix_openpt() opens /dev/ptmx and then confirms devpts is mounted before handing the master back. statfs did not identify /dev/pts, so glibc closed a working master and every glibc pty consumer failed -- terminal emulators, script(1), expect, tmux, screen, sshd, anything using openpty() or forkpty().
Report devpts from statfs and fstatfs, following the per-filesystem pattern already used for proc and tmpfs.
That exposed two more links in the same chain. grantpt(3) chmods the slave when it thinks the mode is wrong, and /dev/pts/N is virtualized for open and stat but not for chmod, so the call fell through to the host and failed with ENOENT. glibc then fell back to the pt_chown helper, which no modern distro ships. Intercept chmod and chown on a live slave instead, where the mode and owner come from the pty layer.
The synthesized slave stat also reported the host file owner rather than the guest uid, which is why grantpt wanted to chown at all. Report the opening uid and the tty group, matching what devpts gives.
tests/test-devpts.c covers the statfs magic, posix_openpt, fstatfs agreement, the grantpt sequence, a master/slave round trip and a negative control that an ordinary path is not devpts. The round trip runs in raw mode under an alarm so a stuck pty fails the test rather than hanging the run.
Fix #263
Summary by cubic
Fixes Unix98 pty allocation under a sysroot by reporting
devptsvia statfs and intercepting chmod/chown on virtual pty slaves. glibc tools usingposix_openpt()/openpty()(tmux, screen, sshd, etc.) work again.Bug Fixes
statfson/dev/ptsand/dev/ptmxnow returnsDEVPTS_SUPER_MAGIC; only live/dev/pts/Nclaimsdevpts; nofstatfsspoof for master fds (matches Linux)./dev/pts/N:statshows the opening guest uid, gidtty(5), mode0620;fchmodatis accepted (no-op);fchownataccepts no-op only and rejects ownership changes.Tests
tests/test-devpts.ccovering devpts statfs,posix_openpt+grantpt/unlockpt/ptsname, master/slave round trip with timeout, ENOENT on bogus paths, and chown semantics; wired intotests/test-matrix.sh.Written for commit eb70a6c. Summary will update on new commits.