sudo-rs icon indicating copy to clipboard operation
sudo-rs copied to clipboard

Problems with piping into sudo-rs

Open DriedSnow opened this issue 5 months ago • 3 comments

Describe the bug When using sudo-rs in a command to remove orphens on Archlinux pipeing pacman -Qqtd in to sudo-rs pacman -Rsu - does not let you enter Y or N to continue and just exits.

To Reproduce Steps to reproduce the behavior:

  1. Install sudo-rs
  2. Run pacman -Qqtd | sudo-rs pacman -Rsu -
  3. see error pacman -Qqtd | sudo-rs pacman -Rsu -

Expected behavior A clear and concise description of what you expected to happen.

Environment (please complete the following information):

  • Linux distribution: Archlinux
  • sudo-rs commit hash: b57be94 and 5314990

Additional context

pacman -Qqtd | sudo-rs pacman -Rsu - checking dependencies... :: binutils optionally requires debuginfod: for debuginfod server/client functionality :: python-geopy optionally requires python-pytz: for timezone support :: python-jinja optionally requires python-babel: for i18n support :: python-mako optionally requires python-babel: for i18n features :: python-pyqt5 optionally requires qt5-location: QtLocation, QtPositioning :: python-pyqt5 optionally requires qt5-webchannel: QtWebChannel :: systemd optionally requires libmicrohttpd: systemd-journal-gatewayd and systemd-journal-remote

Packages (51) cjson-1.7.18-1 debuginfod-0.193-5 libmicrohttpd-1.0.2-1 libx86emu-3.7-2 perl-xml-writer-0.900-5 python-babel-2.17.0-1 python-imagesize-1.4.1-6 python-pytz-2025.2-1 python-roman-numerals-py-3.1.0-1 python-snowballstemmer-2.2.0-7 python-sphinx-8.2.3-1 python-sphinx-alabaster-theme-1.0.0-4 python-sphinxcontrib-applehelp-2.0.0-3 python-sphinxcontrib-devhelp-2.0.0-4 python-sphinxcontrib-htmlhelp-2.1.0-3 python-sphinxcontrib-jquery-4.1-4 python-sphinxcontrib-jsmath-1.0.1-19 python-sphinxcontrib-qthelp-2.0.0-3 python-sphinxcontrib-serializinghtml-2.0.0-3 qt5-location-5.15.17+kde+r7-1 qt5-webchannel-5.15.17+kde+r3-1 asio-1.34.2-1 boost-1.88.0-3 cbindgen-0.29.0-1 check-0.15.2-3 ckbcomp-1.242-1 electron35-35.7.5-1 ffnvcodec-headers-13.0.19.0-1 granite-1:6.2.0-1 hwinfo-25.0-2 kitemmodels5-5.116.0-1 librist-0.2.11-1 libspng-0.7.4-2 libtar-1.2.20-7 libva-utils-2.22.0-1 mbedtls2-2.28.10-1 patchutils-0.4.2-3 profile-sync-daemon-1:6.50-2 python-ply-3.11-15 python-pyaml-24.12.0-1 python-pyliblo-0.10.0-14 python-sphinx-hawkmoth-0.21.0-1 python-sphinx_rtd_theme-2.0.0-2 qrcodegencpp-cmake-1.8.0-4 qt5-webengine-5.15.19-2 rust-bindgen-0.72.0-1 spirv-headers-1:1.4.321.0-1 valgrind-3.25.1-3 vdpauinfo-1.5-2 webkit2gtk-2.48.6-1 xmlto-0.0.29-1

Total Removed Size: 914.19 MiB

:: Do you want to remove these packages? [Y/n] %

DriedSnow avatar Sep 09 '25 07:09 DriedSnow

Thanks for the report! This is the behaviour that I would naively expect. To gather information for deciding on the priority of this bug, we are curious whether this usage does work with original sudo? (I'm not on Arch Linux machine at the moment but a quick check on Ubuntu shows sudo-rs and sudo behaving similarly, and not unexpectedly so, in a comparable situation with apt-get, but it might be that pacman of course implements its prompt differently).

squell avatar Sep 09 '25 14:09 squell

Thanks for the report! This is the behaviour that I would naively expect. To gather information for deciding on the priority of this bug, we are curious whether this usage does work with original sudo? (I'm not on Arch Linux machine at the moment but a quick check on Ubuntu shows sudo-rs and sudo behaving similarly, and not unexpectedly so, in a comparable situation with apt-get, but it might be that pacman of course implements its prompt differently).

It does work the the original sudo properly, letting me press Y or N to continue the command

DriedSnow avatar Sep 09 '25 19:09 DriedSnow

If it helps here is a video of me running both the commands "pacman -Qqtd|sudo pacman -Rsu -" and "pacman -Qqtd|sudo-rs pacman -Rsu -"

https://github.com/user-attachments/assets/5907a8a0-8fc2-47dd-a331-b7dbbf3b14ea

DriedSnow avatar Sep 09 '25 22:09 DriedSnow

Hi, recently i looked at this bug. From what i understood from the code seems like dev/tty is not available as a prompt if stdin is piped since the process is considered as working in background. I would like to work on the issue if it is possible. Regardless of that i will post a few things i discovered. I hope it can be useful to someone else in case.

src/exec/use_pty/parent.rs:107

    if !io::stdin().is_terminal() {
        dev_info!("stdin is not a terminal, command will inherit it");
        pipeline = true;
        command.stdin(Stdio::inherit());

        if foreground && parent_pgrp != sudo_pid {
            // If sudo is not the process group leader and stdin is not a terminal we might be
            // running as a background job via a shell script. Starting in the foreground would
            // change the terminal mode.
            exec_bg = true;
        }
    }

In the snippest above pipeline is set to true and also maybe exec_bg

src/exec/use_pty/parent.rs:182

        match exec_monitor(
            pty.follower,
            command,
            foreground && !pipeline && !exec_bg,
            &mut backchannels.monitor,
            original_set,
        ) {

Because of the previous flags (pipeline, exec_bg) the process is considered as a background process even if, as in the case of the issue, we need the input from /dev/tty.

The % character in the example, from what i understood, is the EOF of the stdin pipe which is used as fallback from pacman since it has no access to the /dev/tty (user input).

I already tested my theory and if we force the execution of the process as foreground

        match exec_monitor(
            pty.follower,
            command,
            true,
            //foreground && !pipeline && !exec_bg,
            &mut backchannels.monitor,
            original_set,
        ) {

the problem is not present for the example above.

I tried a solution for the problem and all the tests are passing but i would like to hear someone who knows the argument more than me.

src/exec/use_pty/parent.rs:107

    if !io::stdin().is_terminal(){
        command.stdin(Stdio::inherit());
        if !fs::File::open("/dev/tty").is_ok() {
            dev_info!("stdin is not a terminal, command will inherit it");
            pipeline = true;

            if foreground && parent_pgrp != sudo_pid {
                // If sudo is not the process group leader and stdin is not a terminal we might be
                // running as a background job via a shell script. Starting in the foreground would
                // change the terminal mode.
                exec_bg = true;
            }
        }
    }

What i'm doing whit this little snippet of code is adding another check which keep the process in foreground even if the stdin is not a tty. Only if /dev/tty is no accessible/closed i consider everything as pipeline or exec_bg. I tested the the suggested snippest and it passes all the tests. So all of that to say that i would like to work on the issue to find the solution to problem. I'm not suggesting the snippest as a solution since it's more an hack which solves the issue.

I hope it can be useful. I'm open to questions and i'm sorry if my description in imprecise but i never work on the sudo code and i'm not really too familiar with it.

mrmonopoly-cyber avatar Nov 16 '25 21:11 mrmonopoly-cyber

The issue is indeed caused because sudo starts the command in the background in this situation and then doesn't make the TTY available.

The fix here that I expect is that as soon as the process does want to read from the TTY, sudo-rs will receive the SIGTTIN signal and then should indeed make the TTY available to the child process.

But if we do that too early you could end up in a similar situation to https://github.com/trifectatechfoundation/sudo-rs/issues/1260

Note that if moving the Stdio::inherit() call in the way you describe still lets sudo-rs pass all integration tests in test-framework/, it would also be useful to us to try and write one where it does behave differently, so that's also a way you could help even if you don't know where to begin with the above description. Most integration tests are testing single invocations and have a bias towards testing security features (for obvious reasons).

squell avatar Nov 18 '25 11:11 squell

Thank you for the answer, i was already thinking on making a test for this situation. For this reason i'm looking on the test framework you are using. Also i understand what you are saying and it makes sense so i will start working on it, if it's not a problem, pretty soon.

mrmonopoly-cyber avatar Nov 19 '25 09:11 mrmonopoly-cyber

Happy hacking! At some point we'll want to have this fixed, but we won't have time before december and more likely january. But I'll notify you if we start taking a stab ourselves to avoid wasting your time.

squell avatar Nov 19 '25 14:11 squell