cargo-credential: reset stdin & stdout to the Console #12469
Conversation
arlosi
added
the
A-credential-provider
|
r? @weihanglo (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-registry-authentication
| let _guard = ReplacementGuard::new(Stdio::Stdin, &mut file).unwrap(); | ||
| let line = std::io::stdin().lines().next().unwrap().unwrap(); | ||
| assert_eq!(line, "hello"); | ||
| } |
There was a problem hiding this comment.
Is it possible to verify that the previous fd is really put back?
There was a problem hiding this comment.
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.
|
Thanks Arlo! @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 21 commits in d78bbf4bde3c6b95caca7512f537c6f9721426ff..7e9de3f4ec3708f500bec142317895b96131e47c 2023-08-03 12:58:25 +0000 to 2023-08-13 00:47:32 +0000 - feat: remove `--keep-going` from `cargo test/bench` (rust-lang/cargo#12478) - chore: window-sys should be a platform-specific dependency (rust-lang/cargo#12483) - docs: make the env var source of rerun-if-env-changed clearer (rust-lang/cargo#12482) - doc: note the backward compatible `.cargo/credential` file exists (rust-lang/cargo#12479) - Fix elided lifetime in associated const (rust-lang/cargo#12475) - prompt the use of `--nocapture` flag if `cargo test` process is terminated via a signal. (rust-lang/cargo#12463) - cargo-credential: reset stdin & stdout to the Console (rust-lang/cargo#12469) - Fix cargo remove incorrectly removing used patches (rust-lang/cargo#12454) - chore(gh): Expand update window (rust-lang/cargo#12466) - Fix panic when enabling http.debug for certain strings (rust-lang/cargo#12468) - fix(cli): Make `--help` easier to browse (rust-lang/cargo#11905) - fix: preserve jobserver file descriptors on rustc invocation to get `TargetInfo` (rust-lang/cargo#12447) - refactor: migrate to `tracing` (rust-lang/cargo#12458) - docs: add example for cargo-credential (rust-lang/cargo#12461) - Bail out an error when using cargo:: in custom build script (rust-lang/cargo#12332) - Fix printing multiple warning messages for unused fields in [registries] table (rust-lang/cargo#12439) - Update windows dependencies (rust-lang/cargo#12453) - Rustfmt a let-else statement (rust-lang/cargo#12451) - Add allow(internal_features) (rust-lang/cargo#12450) - Update pretty_env_logger to 0.5 (rust-lang/cargo#12445) - Remove build metadata from libgit2-sys dependency (rust-lang/cargo#12444) r? `@ghost`
| { | ||
| 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))?; |
There was a problem hiding this comment.
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: 101and 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: 101instead 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
-
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) ↩
-
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/nullor it's not a char device, with that cryptic message not telling you that it's/dev/nullthat's missing. ↩
Credential providers run with stdin and stdout piped to Cargo to communicate. This makes it more difficult for providers to do anything interactive. The current workaround is for a provider to use the
cargo_credential::tty()function when reading from the console by re-opening stdin using/dev/ttyorCONIN$.This PR makes the credential provider to re-attach itself to the current console so that reading from stdin and writing to stdout "just works" when inside the
performmethod of the provider. stderr is unaffected since it's not redirected by Cargo.Only the
cargo-credentialcrate is changed. No changes are needed to Cargo.cc #8933