| Summary: | less should fall back to stdout fd | ||
|---|---|---|---|
| Product: | Busybox | Reporter: | Mike Frysinger <vapier> |
| Component: | Other | Assignee: | unassigned |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | busybox-cvs |
| Priority: | P5 | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Host: | Target: | ||
| Build: | |||
(In reply to Mike Frysinger from comment #0) > except this doesn't make sense -- the fd passed to it is a dup from a parent, so any changes busybox makes to the file status flags via fcntl of its copy doesn't impact other processes. when it exits, its fd simply goes away. O_NONBLOCK _is_ shared across processes. There is only one flag which isn't - O_CLOEXEC. In fcntl language, F_SETFL flags are shared, F_SETFD ones (of which there is only one flag) are not. (In reply to Denys Vlasenko from comment #1) i guess i was looking at file descriptor flags then instead of file status flags ignoring that, i don't see a problem here: we've already mangled the active tty with our settings and must restore them in order for things to continue sanely. so we just need to add a file status flags restore to the already existing less_exit code path (which is where all the tty settings are restored upon manual exit or signal termination). Fixed in git, thanks. |
if the active tty is inaccessible by re-opening, then less fails to get the right terminal settings, so it falls back to bb_cat(). this shouldn't be necessary though as we already have the open stdout fd available to us. there's this comment in the code: /* Some versions of less can survive w/o controlling tty, * try to do the same. This also allows to specify an alternative * tty via "less 1<>TTY". * We don't try to use STDOUT_FILENO directly, * since we want to set this fd to non-blocking mode, * and not bother with restoring it on exit. */ except this doesn't make sense -- the fd passed to it is a dup from a parent, so any changes busybox makes to the file status flags via fcntl of its copy doesn't impact other processes. when it exits, its fd simply goes away. you might have to worry about changing things if less created children, but it doesn't currently support the ! command, so that doesn't come up. what scenario is of concern here ? the reason i bring this up is that busybox's less fails to work when both the controlling tty and /dev/tty nodes are inaccessible. this comes up when using something like `su` to change uid from root to nobody (and /dev/pts/# is root:tty with 620 perms), and a tool creates a new session like setsid() (which means opening /dev/tty returns ENXIO). shadow's `su` implementation currently does exactly that: # su -s /bin/sh -c 'echo hi >/dev/tty' - nobody -su: 1: cannot create /dev/tty: No such device or address # su -s /bin/sh -c 'busybox less /proc/filesystems' - nobody <falls through to bb_cat> # su -s /bin/sh -c 'strace -eopen busybox less /proc/filesystems' - nobody open("/dev/pts/19", O_RDONLY) = -1 EACCES (Permission denied) open("/dev/tty", O_RDONLY) = -1 ENXIO (No such device or address) open("/proc/filesystems", O_RDONLY) = 3 i'd imagine an easy fix would be: --- a/miscutils/less.c +++ b/miscutils/less.c @@ -1781,9 +1781,6 @@ int less_main(int argc, char **argv) /* Some versions of less can survive w/o controlling tty, * try to do the same. This also allows to specify an alternative * tty via "less 1<>TTY". - * We don't try to use STDOUT_FILENO directly, - * since we want to set this fd to non-blocking mode, - * and not bother with restoring it on exit. */ tty_name = xmalloc_ttyname(STDOUT_FILENO); if (tty_name) { @@ -1796,7 +1793,7 @@ int less_main(int argc, char **argv) try_ctty: tty_fd = open(CURRENT_TTY, O_RDONLY); if (tty_fd < 0) - return bb_cat(argv); + tty_fd = STDOUT_FILENO; } ndelay_on(tty_fd); kbd_fd = tty_fd; /* save in a global */