Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: piped less swallows keystrokes meant for next shell command #557

Open
Fry-kun opened this issue Aug 6, 2024 · 17 comments
Open

Comments

@Fry-kun
Copy link

Fry-kun commented Aug 6, 2024

With earlier versions of less (e.g. 608):

$ echo -e "hello $(sleep 2) world" | less -FRX
# <type in "ls" as next command before the prompt comes back>
hello  world
$ ls<cursor>

With version 643:

$ echo -e "hello $(sleep 2) world" | less -FRX
# <type in "ls" as next command before the prompt comes back>
hello  world
$ <cursor>

Many users type in the next command while waiting for the previous one to complete. This is especially painful when using source control commands which paginate most of their output. The current workaround is to turn off paging by default.

@gwsw
Copy link
Owner

gwsw commented Aug 7, 2024

Hm, the idea of typing input for the next command before knowing the outcome of a potentially interactive command seems strange to me. If the output of less happened to be more than a screenful, then less would NOT exit due to the -F flag, and any input typed in would be interpreted as less commands, doing something entirely unintended.

The change in behavior is probably due to support for ^X to interrupt a blocked read, but I'm not convinced that it's a regression.

@Fry-kun
Copy link
Author

Fry-kun commented Aug 7, 2024

I guess not necessarily a regression, but a change in a long-term old behavior.
It might be a case of https://xkcd.com/1172/, but I think a nontrivial number of users expect the old behavior.

Here's an example: Mercurial uses less -FRX by default, so all source control commands are guarded from unexpectedly long output (which seems pretty reasonable).
Many subcommands rarely output a screen's worth of data, but may take a second or two.
It feels pretty reasonable to start entering a new command when not expecting the screen to fill... even if sometimes the assumption doesn't hold true and commands are interpreted by less (usually easy enough to back out of)

@smemsh
Copy link

smemsh commented Aug 10, 2024

I do this all the time (type the next command ahead of time before current command finishes, in anticipation of the prompt), but usually only with commands that don't process terminal input themselves. Since less does, I wouldn't necessarily expect my keys to be interpreted by the shell that resumes after less finishes.

That said, I can see where the old behavior could be useful for specific scenarios. And I can remember instances where I did rely on "knowing" the behavior of an interactive program and typing ahead of time. But that would only be when the behavior was known in advance. I don't think running under -F is such a scenario unless you know the exact input length.

Note that earlier ^X support still had the old behavior. I can reproduce the OP-desired behavior with less-590 (Ubuntu22) which seems to work fine to start/stop Follow with F/^X. I bisected, and the new behavior first appears in v611.

@gwsw
Copy link
Owner

gwsw commented Aug 10, 2024

The issue is less needs to read input characters received while reading file input in order to check whether a ctrl-X has been entered. I guess if -F is given, less could defer checking for ctrl-X until after the first screen has been displayed, but that would mean that while reading a slow pipe, it would be impossible to interrupt the input during that first screen (except via ctrl-C which could kill the piping program). It seems to me that retaining the ctrl-X functionality is more important that allowing this problematic typeahead feature. Another approach would be to read the characters but push them back on the tty if they are not ctrl-X, but I don't know of any way to do that, and even if possible it seems that that might cause other problems.

@smemsh
Copy link

smemsh commented Aug 11, 2024

ioctl(TIOCSTI) is available, it might work on already-opened tty descriptor without privileges, I'm not sure.

If CTRL-C were used to cancel, the user probably intends for the whole process group to get SIGINT anyways (like normally in a shell pipeline), so that may not be an actual problem. But deferring seems like a lot of trouble.

It would be good to know if any others were relying on this behavior...

@gwsw
Copy link
Owner

gwsw commented Aug 11, 2024

TIOCSTI doesn't seem usable. It returns EIO unless I run it as root. Even as root, it doesn't return an error, but the characters don't appear in the shell after less exits.

There are obvious security concerns in having a program inject tty input that is read by the shell, so I'd be surprised if there's a way to do this.

@smemsh
Copy link

smemsh commented Aug 12, 2024

TIOCSTI doesn't seem usable. It returns EIO unless I run it as root

That's interesting, I don't get that (ubuntu22). I'm not advocating for or against this btw, and don't anticipate using it myself. But just as an exercise:

#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <termios.h>
#include <sys/ioctl.h>

void
main (void)
{
        int i, ret, fd, inchar;
        char *toinject = "ls";
        size_t len = strlen(toinject);
        struct termios saved, attrs;

        fd = open("/dev/tty", O_RDWR);
        tcgetattr(fd, &attrs);
        memcpy(&saved, &attrs, sizeof(attrs));

        attrs.c_lflag &= ~ICANON;
        attrs.c_cc[VMIN] = 1;
        attrs.c_cc[VTIME] = 0;
        if ((ret = tcsetattr(fd, TCSANOW, &attrs)) == -1)
                perror("tcsetattr");

        puts("enter a character:");
        read(fd, &inchar, 1);
        printf("\nread a char: %c\n", inchar);

        for (i = 0; i < len; i++)
                if ((ret = ioctl(fd, TIOCSTI, toinject + i)) == -1)
                        perror("ioctl");

        tcsetattr(fd, TCSADRAIN, &saved);
}
 $ ./testinject
enter a character:
a
read a char: a
ls
 $ ls

This was just as an unprivileged user. Worked first try so I didn't try fiddling with flags or anything to see what parts are necessary...

There are obvious security concerns in having a program inject tty input that is read by the shell

I don't think it matters, because any program running with the user's privileges can already do stuff as the user like run execve(2).

@gwsw
Copy link
Owner

gwsw commented Aug 12, 2024

Interesting. Running it on my Fedora system (Linux 6.9.9-200.fc40.x86_64), I get this:

$ ./a.out 
enter a character:
a
read a char: a
ioctl: Input/output error
ioctl: Input/output error

And if I run it as root, I get

$ sudo ./a.out 
enter a character:
a
read a char: a
ls$ 
$ 

Unlike in your output, I don't see "ls" appear after the prompt. Pressing ENTER at that point does not actually execute an "ls" command. Does it in your case?

I don't think it matters, because any program running with the user's privileges can already do stuff as the user like run execve(2).

Yeah, good point.

@smemsh
Copy link

smemsh commented Aug 13, 2024

Yes it definitely queues it on my ubuntu22 system (6.5.0 kernel) and I can just press enter after it exits, and get a listing. That's interesting... I noticed my /dev/tty permissions are 0666, I wonder if yours are? But not working even as root is really interesting. Must be some more paranoid setting on Fedora kernels or something.

Obviously it can't be used if not broadly applicable... well, worth the discussion anyways, and here for posterity now.

@spectral54
Copy link

I just encountered this with jj (http://github.com/martinvonz/jj).

I was also able to reproduce with git blame on a file that had significant history, but fewer lines than $LINES (I wasn't able to reproduce with git log -1, presumably because the time window for me typing between git starting the pager and git populating the output was too small to reliably reproduce; git blame takes longer). Can you tell I work on version control stuff? :)

In the cases where the pager is started by the tool (instead of via explicit user action like some_tool | less -FRX), this behavior is surprising. I'm in a terrible habit of running <vcs> <log_command> without even thinking about it. I also keep that log output very short - it only shows "my" stuff, not everyone else's. I use it as a way of re-orienting myself, or getting a hash to copy+paste, etc. It's very rare that I have more than $LINES of output from this command (and if I do, I work to fix that asap). It's also very likely I ran it less than 30 seconds ago, and know that it'll still fit on my screen. These commands also have a very wide range of execution time (git is typically very fast, hg is typically much slower, at least in our environments), which may be why hg was reported first.

I don't know how common it is for tools other than these VCS tools to start the pager themselves, especially with the -F and -X options. If it's uncommon, maybe we add an environment variable for "less doesn't consume tty input when -F is specified until after it's taking over", as a way of restoring the previous behavior? I'm not confident I understood the change that was made that caused this issue to show up, though.

@smemsh
Copy link

smemsh commented Sep 1, 2024

[@gwsw had run the TIOCSTI example and gotten]:

$ ./a.out 
enter a character:
a
read a char: a
ioctl: Input/output error
ioctl: Input/output error

@gwsw maybe it can be explained by this (system default here):

 $ sysctl dev.tty.legacy_tiocsti
dev.tty.legacy_tiocsti = 1

@gwsw
Copy link
Owner

gwsw commented Sep 1, 2024

Yes, I get a different result from that command:

$ sysctl dev.tty.legacy_tiocsti
dev.tty.legacy_tiocsti = 0

If I change it to 1 and then run the test program, it now works as you describe. I installed this system (Fedora) less than a month ago and I'm certain that I never manually changed that value to 1, so it must have been the default for this system.

I see that the help for that sysctl entry (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=83efeeeb3d04b22aaed1df99bc70a48fe9d22c4d) says

Historically the kernel has allowed TIOCSTI, which will push
characters into a controlling TTY. This continues to be use
as a malicious privilege escalation mechanism, and provides no
meaningful real-world utility any more. Its use is considered
a dangerous legacy operation, and can be disabled on most
systems.

So it seems that it would not be safe to assume that legacy_tiocsti is set on an arbitrary system.

@spectral54
Copy link

Is there anything we can do to work around this issue, such as an environment variable that git, hg, jj, and any others can set that disables the new behavior, or maybe less could automatically disable the behavior when using -F or something like that?

@bradleymoore111
Copy link

bradleymoore111 commented Oct 4, 2024

+1 to the XKCD comment; a lot of developer workflows involve chaining several commands together in rapid-fire, and some of those commands display informational outputs that I don't need. Seeing this information in an information-only manner quickly would be nice, and having a flag we can pass to specify "Don't eat characters or else" would be nice, otherwise we'll have to use something other than less

gwsw added a commit that referenced this issue Oct 4, 2024
@gwsw
Copy link
Owner

gwsw commented Oct 4, 2024

I've added the --no-poll option in 4e516a1. Let me know if that does what you want. Note that less -F will still stop and eat input if the input happens to be longer than the screen height, as it has always done.

@spectral54
Copy link

Thank you! The concern I have with this being a CLI option, however, is that we need to either version-detect less to know it has it, or we can only set it in managed environments where we know less has it (because we centrally manage package installations on the user's machines). This is why I was hoping for the environment variable approach: this way, hg, jj, git, and anything else using less -F can just set it in the environment and forget about it. less won't read from it unless it's new enough, no warning messages about unknown flags, etc.

@gwsw
Copy link
Owner

gwsw commented Oct 8, 2024

You can use the LESSKEY_CONTENT environment variable to conditionally set command line options based on the version. Unfortunately the post659 branch doesn't yet have a version number, but suppose the --no-poll option appears in the master branch in version 670. Then you could put this in your environment:

LESSKEY_CONTENT='#env;#version>=670 LESS=${LESS} --no-poll'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants