Less fails to read pipe from sudo if sudo prompts for a password

[rpigott]

The following reproduces the issue somewhat reliably in both zsh and bash when sudo is configured to promprt for a password:

sudo -k seq 100 | less

We expect less to display the integers 1..100 but attempting to enter the sudo passphrase usually results in getting stuck "Waiting for data...".

This is a regression. I bisected it to 953257f:

git bisect log

git bisect start
# status: waiting for both good and bad commits
# good: [22e4af5cccbfab633000c7d610f868a868ad6e1a] v608
git bisect good 22e4af5cccbfab633000c7d610f868a868ad6e1a
# status: waiting for bad commit, 1 good commit known
# bad: [0d7ca880375bde651313f03f7115d26921b43722] v633
git bisect bad 0d7ca880375bde651313f03f7115d26921b43722
# bad: [fb09748090dfa58de138978a8a06b8acafc48727] Cleanup
git bisect bad fb09748090dfa58de138978a8a06b8acafc48727
# bad: [fb09748090dfa58de138978a8a06b8acafc48727] Cleanup
git bisect bad fb09748090dfa58de138978a8a06b8acafc48727
# good: [ece2600cb0f0c8339cfe00ffa8e2dbc28df38622] v609
git bisect good ece2600cb0f0c8339cfe00ffa8e2dbc28df38622
# bad: [a03f8263a009b5e60aff50e438f08faf7daaa3a2] Open v613.
git bisect bad a03f8263a009b5e60aff50e438f08faf7daaa3a2
# bad: [1216c2baa3ccbafe0367edf06209e62d170536ff] Files genrated from updated Unicode data.
git bisect bad 1216c2baa3ccbafe0367edf06209e62d170536ff
# good: [ab5ad29445368e7da91a80d433a3f88e42be6127] Merge branch 'master' of https://github.com/gwsw/less
git bisect good ab5ad29445368e7da91a80d433a3f88e42be6127
# bad: [112b028f1421f093e9f79600fb67e6a78ccdd414] Add unicode-check target.
git bisect bad 112b028f1421f093e9f79600fb67e6a78ccdd414
# bad: [8e3ec369acfa1741c2e84a1655914c40c63a3f47] More reasonable number.
git bisect bad 8e3ec369acfa1741c2e84a1655914c40c63a3f47
# bad: [953257f9030d29dd930ed3e76fd5811681b672b1] Improve ability of ^X to interrupt F.
git bisect bad 953257f9030d29dd930ed3e76fd5811681b672b1
# first bad commit: [953257f9030d29dd930ed3e76fd5811681b672b1] Improve ability of ^X to interrupt F.

[gwsw]

Well, this is unfortunate. It's kind of an accident that this ever worked, since in principle sudo and less are both trying to read from the terminal at the same time. In older versions, less didn't actually access the terminal until the first page of output appeared, thus implicitly allowing sudo to use the terminal first, and since sudo uses the terminal only briefly things looked ok. If sudo were replaced with some interactive program that used the terminal over a longer period of time, there would of course be contention between that program and (any version of) less. No good solutions occur to me right now, but I will give this some thought.

[tzcrawford]

I'm having a similar issue running dictd output into less within a rxvt-unicode subshell.

The command urxvt -e sh -c "dict -d english 'blue' | less " produces an empty less window (no content/empty string).
Although urxvt -e sh -c "dict -d english 'blue' | more " works as expected, and so does dict -d english 'blue' | less and urxvt -e sh -c "echo -e 'foo foo\n bar' | less". So just running the three programs in conjuction is the issue. Running either dict or urxvt alone with less is working.

This bug started occuring with v633 on the Arch Linux build, although v608 was fine.

[rpigott]

In older versions, less didn't actually access the terminal until the first page of output appeared, thus implicitly allowing sudo to use the terminal first

fwiw I think this is pretty reasonable behavior, though I'm not familiar with the issue that the offending commit claims to be addressing. Maybe if we revert that commit, there is a more reasonable fix? Maybe we can poll until the paged file is readable and just choose not to read from the terminal until then. I don't quite understand the rationale for the change, but you could always just choose to read from the terminal first once both fd are readable.

[gwsw]

Ok, I've made a tentative change in 5e93b7b for testing. It avoids printing the "waiting for data" message if no data has yet been received. After the first byte has been received, then it reverts to the behavior in less-633. This seems to allow sudo to work as it did in previous releases. I haven't tried the urxvt/dict case.

[michaelbrunnbauer]

I have the problem with pinentry-curses/pinentry-tty from gnupg. This patch does not solve it for me.

[tzcrawford]

My build at 5e93b7b is still performing the urxvt/dict issue from my previous comment.

[gwsw]

@tzcrawford The urxvt/dict issue may not be related to this issue. Your urxvt command actually works fine for me on a Fedora 5.12.8-300.fc34.x86_64 system, either with less-633 or with the 5e93b7b patch. What environment are you running on?

@michaelbrunnbauer Can you give instructions for reproducing the pinentry issue you are seeing?

[michaelbrunnbauer]

@gwsw Calling gpg --decrypt on a file and piping the output into less. gpg is configured to use pinentry-curses to ask for the password (pinentry-program /usr/bin/pinentry-curses in .gnupg/gpg-agent.conf).

[tzcrawford]

@tzcrawford The urxvt/dict issue may not be related to this issue. Your urxvt command actually works fine for me on a Fedora 5.12.8-300.fc34.x86_64 system, either with less-633 or with the 5e93b7b patch. What environment are you running on?

Right now I am using

less 1:633-1
rxvt-unicode 9.31-2 (9.26-3 appears to be the same)
rxvt-unicode-terminfo 9.31-2
dictd 1.13.1-4
linux 6.3.4-arch1-1 (Arch Linux build)
bash 5.1.16
perl 5.36.1-1 (under urxvt)
on an Acer Aspire E5-521 V1.03 laptop, AMD E2-6110 APU

Side note, with xterm 380-1, I get the following command to execute as it should
xterm -e "dict -d english 'blue' | less"

Should I make a separate issue to track this?

[gwsw]

@michaelbrunnbauer The patch seems to fix the pinentry issue for me. If I edit ~/.gnupg/gpg-agent.conf as you describe and run gpg --decrypt file.gpg | less using less-633, then the "waiting for data" message appears while I'm trying to enter the passphrase. But if I use less-634x (with the 5e93b7b patch), I don't see the "waiting for data" message, I am able to enter the passphrase, and the decrypted file appears in less as expected.

Can you confirm that you're using less-634x and if so, describe the behavior that you're seeing?

[rpigott]

@gwsw Thanks for the patch, but that is not sufficient. I can still reproduce.

We can't call getc if we don't want to disturb other readers of the pty, since we can't ungetc characters back to those other readers. IMO less should only read from tty if the input has already reported POLLIN or POLLHUP, but I think that is more or less equivalent to the old behavior where less issued a blocking read on the input when neither were readable. I don't really understand the motivation for changing the behavior.

[gwsw]

The purpose of the change was to allow ^X to interrupt a slow input pipe and return to the command prompt. So before reading from the input file, it polls both the input file and the tty, and only reads from the input file if data is available, or if a tty char is available it reads that and determines if it a ^X.

I see that there was a logic error in my previous patch. Can you try fd2a746 and see if that fixes it for you?

[michaelbrunnbauer]

@michaelbrunnbauer The patch seems to fix the pinentry issue for me. If I edit ~/.gnupg/gpg-agent.conf as you describe and run gpg --decrypt file.gpg | less using less-633, then the "waiting for data" message appears while I'm trying to enter the passphrase. But if I use less-634x (with the 5e93b7b patch), I don't see the "waiting for data" message, I am able to enter the passphrase, and the decrypted file appears in less as expected.

Can you confirm that you're using less-634x and if so, describe the behavior that you're seeing?

I was using less-633 with the 5e93b7b patch and saw the "waiting for data" message. Does less-634x have any other changes? If yes, where do I get it?

[gwsw]

There was an error in the 5e93b7b patch. Can you update to the fd2a746 patch and try again?

[michaelbrunnbauer]

There was an error in the 5e93b7b patch. Can you update to the fd2a746 patch and try again?

No more error message and I can enter the password. But the first try always fails - pinentry seems to get extra data.

[rpigott]

if a tty char is available it reads that and determines if it a ^X.

That still interferes with other readers. With fd2a746 less still races a blocking read against sudo, so the password input is sometimes read incorrectly by sudo. Even if we could stuff it back into the tty (and we can't without TIOCSTI), it's probably preferable that less doesn't read the password. We shouldn't race other readers. If they take the control inputs from us that's fine — probably intended even.

The purpose of the change was to allow ^X to interrupt a slow input pipe and return to the command prompt.

Here's my idea: if what you don't want is for blocking read on input to defer control inputs, try racing poll against read on the tty instead so other readers get inputs first if they are present, and only issue non-blocking reads to the tty, so we don't end up accidentally grabbing the next available inputs. I made this example program:

reader.c

#include <errno.h>
#include <ctype.h>
#include <fcntl.h>
#include <poll.h>
#include <stdio.h>
#include <termios.h>
#include <unistd.h>

#define LEN(x) (sizeof(x) / sizeof(x[0]))
#define BUFSZ 4096

int main(int argc, char *argv[]) {
    int tty = open("/dev/tty", O_RDONLY | O_NOCTTY | O_NONBLOCK);

    struct termios termios_p;
    tcgetattr(tty, &termios_p);
    termios_p.c_lflag &= ~(ICANON | ECHO);
    tcsetattr(tty, TCSADRAIN, &termios_p);

    enum pollfds {
        FILE_STDIN,
        FILE_TTY,
    };

    struct pollfd fds[] = {
        [FILE_STDIN] = {0  , POLLIN, 0},
        [FILE_TTY  ] = {tty, POLLIN, 0},
    };

    char buf[BUFSZ], c;
    int ret = 0;
    while ((ret = poll(fds, LEN(fds), -1)) != -1) {
        if (fds[FILE_TTY].revents & POLLIN) {
            int n = read(tty, &c, 1);
            if (errno == EAGAIN) {
                // that's fine, someone else got it
                errno = 0;
            } else {
                printf("tty: '%s%c'\n", isprint(c) ? "" : "^", isprint(c) ? c : (c | 0x40));
            }
        }
        if (fds[FILE_STDIN].revents & POLLIN) {
            int n = read(0, buf, BUFSZ-1); buf[n] = '\0';
            printf("%s", buf);
        }
        if (fds[FILE_STDIN].revents & (POLLHUP|POLLERR) ||
            fds[FILE_TTY].revents   & (POLLHUP|POLLERR)) {
            break;
        }
    }
}

I simulate the example of sudo with a slow output like so:

$ cc -o ./reader reader.c
$ sudo -k bash -c 'sleep 3 && seq 5' | ./reader
[sudo] password for ronan: 
tty: '^X'
tty: '^X'
tty: '^X'
1
2
3
4
5

I input my password, then hit ctrl-x a few times whlie bash is sleeping before the input is available. Here sudo reads the password correctly and ./reader gets the control inputs before stdin has anything available to read (as long as there is no other reader on $TTY). I tried adapting this to less's codebase but struggled with the iread/check_poll control flow.

[rpigott]

No more error message and I can enter the password. But the first try always fails - pinentry seems to get extra data.

@michaelbrunnbauer less is stealing some of the password characters

[gwsw]

Ok, I agree that less is stealing tty chars from the piping program. I think to avoid this (according to my plan of deferring polling behavior until input is received), check_poll should just do nothing and return 0 if any_data is false. c8df315 should fix that.

I'm interested in your idea of fixing this without needing to defer until input is received, but I don't quite understand how your reader program does that. If tty data is available, then your poll should set fds[FILE_TTY].revents to POLLIN, so why wouldn't your read(tty) sometimes steal the char? Unless I'm missing something, that is pretty much was less-633 was doing.

[rpigott]

@gwsw The theory at least is that our read always comes after any blocking read, with the assumption that any new read call won't get data that another read is already waiting for. Previously less is racing it's own read against another read and which read was issued first also depends on the order the shell spawns our processes in the pipeline.

I don't actually know if this is still a race. Maybe it is and I'm just intentionally losing it, but it does seem to work on my laptop at least. Obviously it could still lose if sudo polled the tty for input as well.

[gwsw]

Ok, I see. It seems a bit risky to me, so I think I'd rather stick with the current design. I don't think it's particularly useful to ^X out of the very first read, when no data is shown yet. The point of ^X is to be able to regain control so you can poke around and look at the currently displayed data.

Could everyone in this thread (@michaelbrunnbauer @rpigott @tzcrawford) confirm whether c8df315 works as expected? @tzcrawford I do suspect your issue is unrelated, so if you still see it, please open a new bug report.

[rpigott]

That's fine by me. I'm not really bothered if a slow input defers tty input, so I don't mind if we wait. I just wanted to offer a solution that might appease all parties. The behavior in the OP does not reproduce for me on c8df315, and sudo | less works as desired. Feel free to close this issue.

[michaelbrunnbauer]

Could everyone in this thread (@michaelbrunnbauer @rpigott @tzcrawford) confirm whether c8df315 works as expected?

gpg --decrypt <file> | less works as in earlier versions now. Thank you!

[tzcrawford]

Could everyone in this thread (@michaelbrunnbauer @rpigott @tzcrawford) confirm whether c8df315 works as expected? @tzcrawford I do suspect your issue is unrelated, so if you still see it, please open a new bug report.

I can confirm my issue was not solved at this commit. I will try to file a new issue later today.