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

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Aug 9, 2023

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/tty or CONIN$.

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 perform method of the provider. stderr is unaffected since it's not redirected by Cargo.

Only the cargo-credential crate is changed. No changes are needed to Cargo.

cc #8933

@arlosi arlosi added the A-credential-provider Area: credential provider for storing and retreiving credentials label Aug 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 9, 2023

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-registry-authentication Area: registry authentication and authorization (authn authz) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2023
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks pretty nice! Just a few questions.

credential/cargo-credential/Cargo.toml Outdated Show resolved Hide resolved
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.

@weihanglo
Copy link
Member

Thanks Arlo!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 10, 2023

📌 Commit 5ade1ad has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2023
@bors
Copy link
Contributor

bors commented Aug 10, 2023

⌛ Testing commit 5ade1ad with merge 54550ef...

@bors
Copy link
Contributor

bors commented Aug 10, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 54550ef to master...

@bors bors merged commit 54550ef into rust-lang:master Aug 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2023
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`
@ehuss ehuss added this to the 1.73.0 milestone Aug 22, 2023
{
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-credential-provider Area: credential provider for storing and retreiving credentials A-registry-authentication Area: registry authentication and authorization (authn authz) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants