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

cargo-credential: reset stdin & stdout to the Console #12469

Merged
merged 1 commit into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 1 addition & 6 deletions credential/cargo-credential-1password/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ impl OnePasswordKeychain {
let mut cmd = Command::new("op");
cmd.args(["signin", "--raw"]);
cmd.stdout(Stdio::piped());
cmd.stdin(cargo_credential::tty().map_err(Box::new)?);
let mut child = cmd
.spawn()
.map_err(|e| format!("failed to spawn `op`: {}", e))?;
Expand Down Expand Up @@ -210,7 +209,7 @@ impl OnePasswordKeychain {
Some(name) => format!("Cargo registry token for {}", name),
None => "Cargo registry token".to_string(),
};
let mut cmd = self.make_cmd(
let cmd = self.make_cmd(
session,
&[
"item",
Expand All @@ -225,10 +224,6 @@ impl OnePasswordKeychain {
CARGO_TAG,
],
);
// For unknown reasons, `op item create` seems to not be happy if
// stdin is not a tty. Otherwise it returns with a 0 exit code without
// doing anything.
cmd.stdin(cargo_credential::tty().map_err(Box::new)?);
self.run_cmd(cmd)?;
Ok(())
}
Expand Down
2 changes: 2 additions & 0 deletions credential/cargo-credential/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ description = "A library to assist writing Cargo credential helpers."

[dependencies]
anyhow.workspace = true
libc.workspace = true
serde = { workspace = true, features = ["derive"] }
serde_json.workspace = true
thiserror.workspace = true
time.workspace = true
windows-sys = { workspace = true, features = ["Win32_System_Console", "Win32_Foundation"] }

[dev-dependencies]
snapbox = { workspace = true, features = ["examples"] }
25 changes: 25 additions & 0 deletions credential/cargo-credential/examples/stdout-redirected.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//! Provider used for testing redirection of stdout.

use cargo_credential::{Action, Credential, CredentialResponse, Error, RegistryInfo};

struct MyCredential;

impl Credential for MyCredential {
fn perform(
&self,
_registry: &RegistryInfo,
_action: &Action,
_args: &[&str],
) -> Result<CredentialResponse, Error> {
// Informational messages should be sent on stderr.
eprintln!("message on stderr should be sent the the parent process");

// Reading from stdin and writing to stdout will go to the attached console (tty).
println!("message from test credential provider");
Err(Error::OperationNotSupported)
}
}

fn main() {
cargo_credential::main(MyCredential);
}
35 changes: 11 additions & 24 deletions credential/cargo-credential/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,16 @@
//! ```

use serde::{Deserialize, Serialize};
use std::{
fmt::Display,
fs::File,
io::{self, BufRead, BufReader},
};
use std::{fmt::Display, io};
use time::OffsetDateTime;

mod error;
mod secret;
mod stdio;

pub use error::Error;
pub use secret::Secret;
use stdio::stdin_stdout_to_console;

/// Message sent by the credential helper on startup
#[derive(Serialize, Deserialize, Clone, Debug)]
Expand Down Expand Up @@ -241,32 +240,20 @@ fn doit(
if request.v != PROTOCOL_VERSION_1 {
return Err(format!("unsupported protocol version {}", request.v).into());
}
serde_json::to_writer(
std::io::stdout(),
&credential.perform(&request.registry, &request.action, &request.args),
)?;

let response = stdin_stdout_to_console(|| {
credential.perform(&request.registry, &request.action, &request.args)
})?;

serde_json::to_writer(std::io::stdout(), &response)?;
println!();
}
}

/// Open stdin from the tty
pub fn tty() -> Result<File, io::Error> {
#[cfg(unix)]
const IN_DEVICE: &str = "/dev/tty";
#[cfg(windows)]
const IN_DEVICE: &str = "CONIN$";
let stdin = std::fs::OpenOptions::new()
.read(true)
.write(true)
.open(IN_DEVICE)?;
Ok(stdin)
}

/// Read a line of text from stdin.
pub fn read_line() -> Result<String, io::Error> {
let mut reader = BufReader::new(tty()?);
let mut buf = String::new();
reader.read_line(&mut buf)?;
io::stdin().read_line(&mut buf)?;
Ok(buf.trim().to_string())
}

Expand Down
163 changes: 163 additions & 0 deletions credential/cargo-credential/src/stdio.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
use std::{fs::File, io::Error};

/// Reset stdin and stdout to the attached console / tty for the duration of the closure.
/// If no console is available, stdin and stdout will be redirected to null.
pub fn stdin_stdout_to_console<F, T>(f: F) -> Result<T, Error>
where
F: FnOnce() -> T,
{
let open_write = |f| std::fs::OpenOptions::new().write(true).open(f);

let mut stdin = File::open(imp::IN_DEVICE).or_else(|_| File::open(imp::NULL_DEVICE))?;
Copy link

@correabuscar correabuscar May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both /dev/tty and /dev/null don't exist, you get something like this whereby /dev/null is a file now and some previous output will now be interpreted and fail compilation (or worse).

OR, maybe I got this wrong and the issue is somewhere else, in rustc, for example:
https://github.com/rust-lang/rust/blob/4cf5723dbe471ef0a32857b968b91498551f5e38/library/std/src/sys/pal/unix/process/process_common.rs#L479-L486

EDIT: yep, I got it wrong, SORRY!, it's not your code here that affected the link I gave initially, it's the one in rustc mentioned belowabove(moved up)!

Either way, perhaps a better error reported(even here) would be better? you know, like related to the problem. For example a patch like this, for your code here (aka cargo test stdout_redirected inside ./credential/cargo-credential/ subdir of a git clone of this cargo repo) but also for rustc, would look like this(assuming it already got compiled1, else the patch for rustc will trigger instead thus obsoleting the need for patching this in cargo2):

(when /dev/null is a file because sudo always created an empty 0 byte one if none exists)

    Finished test [unoptimized + debuginfo] target(s) in 0.08s
     Running unittests src/lib.rs (/doo/cargo/target/debug/deps/cargo_credential-d11fc214c5de4ef7)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s

     Running tests/examples.rs (/doo/cargo/target/debug/deps/examples-9d18408056c4cd23)

running 1 test
test stdout_redirected ... FAILED

failures:

---- stdout_redirected stdout ----
thread 'stdout_redirected' panicked at /doo/cargo/credential/cargo-credential/tests/examples.rs:17:10:

--- Expected
++++ actual:   stdout
   1    1 | {"v":[1]}
   2      - {"Err":{"kind":"operation-not-supported"}}
        2 + {"Err":{"kind":"other","message":"The file '/dev/null' is not a character device","caused-by":[]}}

Exit status: 0

stack backtrace:
   0:     0x5a7e3563f57c - <unknown>
   1:     0x5a7e3566db30 - <unknown>
   2:     0x5a7e3563cccd - <unknown>
   3:     0x5a7e3563f354 - <unknown>
   4:     0x5a7e35640f97 - <unknown>
   5:     0x5a7e35640cb3 - <unknown>
   6:     0x5a7e354e8927 - <unknown>
   7:     0x5a7e356415e0 - <unknown>
   8:     0x5a7e35641302 - <unknown>
   9:     0x5a7e3563fa96 - <unknown>
  10:     0x5a7e35641060 - <unknown>
  11:     0x5a7e354af265 - <unknown>
  12:     0x5a7e354f53ef - <unknown>
  13:     0x5a7e3553228e - <unknown>
  14:     0x5a7e355309d7 - <unknown>
  15:     0x5a7e354b1432 - <unknown>
  16:     0x5a7e354b0959 - <unknown>
  17:     0x5a7e354afe57 - <unknown>
  18:     0x5a7e354aff76 - <unknown>
  19:     0x5a7e354ee06f - <unknown>
  20:     0x5a7e354eced4 - <unknown>
  21:     0x5a7e354b4346 - <unknown>
  22:     0x5a7e354b93e7 - <unknown>
  23:     0x5a7e356479c5 - <unknown>
  24:     0x73de49c33ff1 - start_thread
                               at /usr/src/debug/sys-libs/glibc-2.39-r6/glibc-2.39/nptl/pthread_create.c:447:8
  25:     0x73de49cb3afc - __GI___clone3
                               at /usr/src/debug/sys-libs/glibc-2.39-r6/glibc-2.39/misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
  26:                0x0 - <unknown>


failures:
    stdout_redirected

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.09s

error: test failed, to rerun pass `--test examples`
exit code: 101

and when /dev/null doesn't exist:

    Finished test [unoptimized + debuginfo] target(s) in 0.09s
     Running unittests src/lib.rs (/doo/cargo/target/debug/deps/cargo_credential-d11fc214c5de4ef7)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s

     Running tests/examples.rs (/doo/cargo/target/debug/deps/examples-9d18408056c4cd23)

running 1 test
test stdout_redirected ... FAILED

failures:

---- stdout_redirected stdout ----
thread 'stdout_redirected' panicked at /doo/cargo/credential/cargo-credential/tests/examples.rs:17:10:

--- Expected
++++ actual:   stdout
   1    1 | {"v":[1]}
   2      - {"Err":{"kind":"operation-not-supported"}}
        2 + {"Err":{"kind":"other","message":"Error accessing character device '/dev/null': No such file or directory (os error 2)","caused-by":[]}}

Exit status: 0

stack backtrace:
   0:     0x6275f5a8857c - <unknown>
   1:     0x6275f5ab6b30 - <unknown>
   2:     0x6275f5a85ccd - <unknown>
   3:     0x6275f5a88354 - <unknown>
   4:     0x6275f5a89f97 - <unknown>
   5:     0x6275f5a89cb3 - <unknown>
   6:     0x6275f5931927 - <unknown>
   7:     0x6275f5a8a5e0 - <unknown>
   8:     0x6275f5a8a302 - <unknown>
   9:     0x6275f5a88a96 - <unknown>
  10:     0x6275f5a8a060 - <unknown>
  11:     0x6275f58f8265 - <unknown>
  12:     0x6275f593e3ef - <unknown>
  13:     0x6275f597b28e - <unknown>
  14:     0x6275f59799d7 - <unknown>
  15:     0x6275f58fa432 - <unknown>
  16:     0x6275f58f9959 - <unknown>
  17:     0x6275f58f8e57 - <unknown>
  18:     0x6275f58f8f76 - <unknown>
  19:     0x6275f593706f - <unknown>
  20:     0x6275f5935ed4 - <unknown>
  21:     0x6275f58fd346 - <unknown>
  22:     0x6275f59023e7 - <unknown>
  23:     0x6275f5a909c5 - <unknown>
  24:     0x727ddc791ff1 - start_thread
                               at /usr/src/debug/sys-libs/glibc-2.39-r6/glibc-2.39/nptl/pthread_create.c:447:8
  25:     0x727ddc811afc - __GI___clone3
                               at /usr/src/debug/sys-libs/glibc-2.39-r6/glibc-2.39/misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
  26:                0x0 - <unknown>


failures:
    stdout_redirected

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.09s

error: test failed, to rerun pass `--test examples`
exit code: 101

instead of like this(without the patch, that is):

# sudo -u user -- ./g
    Finished test [unoptimized + debuginfo] target(s) in 0.09s
     Running unittests src/lib.rs (/doo/cargo/target/debug/deps/cargo_credential-d11fc214c5de4ef7)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s

     Running tests/examples.rs (/doo/cargo/target/debug/deps/examples-9d18408056c4cd23)

running 1 test
test stdout_redirected ... FAILED

failures:

---- stdout_redirected stdout ----
thread 'stdout_redirected' panicked at /doo/cargo/credential/cargo-credential/tests/examples.rs:17:10:

--- Expected
++++ actual:   stdout
   1    1 | {"v":[1]}
   2      - {"Err":{"kind":"operation-not-supported"}}
        2 + {"Err":{"kind":"other","message":"No such file or directory (os error 2)","caused-by":[]}}

Exit status: 0

stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    stdout_redirected

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.10s

error: test failed, to rerun pass `--test examples`

see? no one knows which file's missing (it's /dev/null but it doesn't say)

Footnotes

  1. and thus the test binary is just being re-run, without needing a re-compilation(because a re-compilation would make it fail in rustc first)

  2. well, except that if u ship that binary(in this case it's just the test itself) then that binary will fail when no /dev/null or it's not a char device, with that cryptic message not telling you that it's /dev/null that's missing.

let mut stdout = open_write(imp::OUT_DEVICE).or_else(|_| open_write(imp::NULL_DEVICE))?;

let _stdin_guard = imp::ReplacementGuard::new(Stdio::Stdin, &mut stdin)?;
let _stdout_guard = imp::ReplacementGuard::new(Stdio::Stdout, &mut stdout)?;
Ok(f())
}

enum Stdio {
Stdin,
Stdout,
}

#[cfg(windows)]
mod imp {
use super::Stdio;
use std::{fs::File, io::Error, os::windows::prelude::AsRawHandle};
use windows_sys::Win32::{
Foundation::{HANDLE, INVALID_HANDLE_VALUE},
System::Console::{
GetStdHandle, SetStdHandle, STD_HANDLE, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE,
},
};
pub const OUT_DEVICE: &str = "CONOUT$";
pub const IN_DEVICE: &str = "CONIN$";
pub const NULL_DEVICE: &str = "NUL";

/// Restores previous stdio when dropped.
pub struct ReplacementGuard {
std_handle: STD_HANDLE,
previous: HANDLE,
}

impl ReplacementGuard {
pub(super) fn new(stdio: Stdio, replacement: &mut File) -> Result<ReplacementGuard, Error> {
let std_handle = match stdio {
Stdio::Stdin => STD_INPUT_HANDLE,
Stdio::Stdout => STD_OUTPUT_HANDLE,
};

let previous;
unsafe {
// Make a copy of the current handle
previous = GetStdHandle(std_handle);
if previous == INVALID_HANDLE_VALUE {
return Err(std::io::Error::last_os_error());
}

// Replace stdin with the replacement handle
if SetStdHandle(std_handle, replacement.as_raw_handle() as HANDLE) == 0 {
return Err(std::io::Error::last_os_error());
}
}

Ok(ReplacementGuard {
previous,
std_handle,
})
}
}

impl Drop for ReplacementGuard {
fn drop(&mut self) {
unsafe {
// Put previous handle back in to stdin
SetStdHandle(self.std_handle, self.previous);
}
}
}
}

#[cfg(unix)]
mod imp {
use super::Stdio;
use libc::{close, dup, dup2, STDIN_FILENO, STDOUT_FILENO};
use std::{fs::File, io::Error, os::fd::AsRawFd};
pub const IN_DEVICE: &str = "/dev/tty";
pub const OUT_DEVICE: &str = "/dev/tty";
pub const NULL_DEVICE: &str = "/dev/null";

/// Restores previous stdio when dropped.
pub struct ReplacementGuard {
std_fileno: i32,
previous: i32,
}

impl ReplacementGuard {
pub(super) fn new(stdio: Stdio, replacement: &mut File) -> Result<ReplacementGuard, Error> {
let std_fileno = match stdio {
Stdio::Stdin => STDIN_FILENO,
Stdio::Stdout => STDOUT_FILENO,
};

let previous;
unsafe {
// Duplicate the existing stdin file to a new descriptor
previous = dup(std_fileno);
if previous == -1 {
return Err(std::io::Error::last_os_error());
}
// Replace stdin with the replacement file
if dup2(replacement.as_raw_fd(), std_fileno) == -1 {
return Err(std::io::Error::last_os_error());
}
}

Ok(ReplacementGuard {
previous,
std_fileno,
})
}
}

impl Drop for ReplacementGuard {
fn drop(&mut self) {
unsafe {
// Put previous file back in to stdin
dup2(self.previous, self.std_fileno);
// Close the file descriptor we used as a backup
close(self.previous);
}
}
}
}

#[cfg(test)]
mod test {
use std::fs::OpenOptions;
use std::io::{Seek, Write};

use super::imp::ReplacementGuard;
use super::Stdio;

#[test]
fn stdin() {
let tempdir = snapbox::path::PathFixture::mutable_temp().unwrap();
let file = tempdir.path().unwrap().join("stdin");
let mut file = OpenOptions::new()
.read(true)
.write(true)
.create(true)
.open(file)
.unwrap();

writeln!(&mut file, "hello").unwrap();
file.seek(std::io::SeekFrom::Start(0)).unwrap();
{
let _guard = ReplacementGuard::new(Stdio::Stdin, &mut file).unwrap();
let line = std::io::stdin().lines().next().unwrap().unwrap();
assert_eq!(line, "hello");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to verify that the previous fd is really put back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this test, since reading from stdin after it's put back will either block forever or fail.

The integration tests in examples.rs do validate that the pervious fd is put back though. If the previous fd was not put back, then the JSON-serialized output would not get back to the parent process, and the stdout_eq asserts would fail.

}
}
17 changes: 17 additions & 0 deletions credential/cargo-credential/tests/examples.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,23 @@ use std::path::Path;

use snapbox::cmd::Command;

#[test]
fn stdout_redirected() {
let bin = snapbox::cmd::compile_example("stdout-redirected", []).unwrap();

let hello = r#"{"v":[1]}"#;
let get_request = r#"{"v": 1, "registry": {"index-url":"sparse+https://test/","name":"alternative"},"kind": "get","operation": "read","args": []}"#;
let err_not_supported = r#"{"Err":{"kind":"operation-not-supported"}}"#;

Command::new(bin)
.stdin(format!("{get_request}\n"))
.arg("--cargo-plugin")
.assert()
.stdout_eq(format!("{hello}\n{err_not_supported}\n"))
.stderr_eq("message on stderr should be sent the the parent process\n")
.success();
}

#[test]
fn file_provider() {
let bin = snapbox::cmd::compile_example("file-provider", []).unwrap();
Expand Down