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

fix: Move off atty to resolve soundness issue #11420

Merged
merged 1 commit into from
Nov 25, 2022
Merged

Conversation

epage
Copy link
Contributor

@epage epage commented Nov 25, 2022

There is a soundness issue with atty when building on Windows with a custom allocator.

This PR switches direct dependencies on atty to is-terminal. New semver compatible versions of clap and snapbox remove atty. #11417 upgrades env_logger to remove it from there.

Fixes #11416

@rustbot
Copy link
Collaborator

rustbot commented Nov 25, 2022

r? @ehuss

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2022
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.

Thanks for the quick patch!

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -16,7 +16,7 @@ name = "cargo"
path = "src/cargo/lib.rs"

[dependencies]
atty = "0.2"
is-terminal = "0.4.0"
Copy link
Member

Choose a reason for hiding this comment

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

Raise the awareness that introducing this dependency means Cargo starts to depend on rustix indirectly. See #10309

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about switching to rustix, and I echo the concerns at #10309 (comment). Do we know how well this will handle the myriad of platforms cargo is used on? I suppose one option is to push it out and see if anyone complains, but I think we should be prepared to revert this if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we can continue using atty for Unix. Only on Windows do we use rustix, which effectively uses libc. Does this sound reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a particular opinion about what to use. Just pointing out to beware of the risks of this change. Maybe @joshtriplett may have some thoughts, or maybe a better sense of when IsTerminal could be stabilized, in which case we could just wait or switch to that soon.

There is a soundness issue with atty when building on Windows with a
custom allocator.

This PR switches direct dependencies on atty to is-terminal.  New semver
compatible versions of clap and snapbox remove atty. rust-lang#11417 upgrades
env_logger to remove it from there.

Fixes rust-lang#11415
@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2022

📌 Commit 48895b1 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 Nov 25, 2022
@bors
Copy link
Contributor

bors commented Nov 25, 2022

⌛ Testing commit 48895b1 with merge e027c4b...

@bors
Copy link
Contributor

bors commented Nov 25, 2022

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

@bors bors merged commit e027c4b into rust-lang:master Nov 25, 2022
weihanglo added a commit to weihanglo/rust that referenced this pull request Nov 25, 2022
5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744
2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000
- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 26, 2022
Update cargo

5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744 2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000

- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)

r+ `@ghost`
weihanglo added a commit to weihanglo/rust that referenced this pull request Nov 27, 2022
5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744
2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000
- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 28, 2022
Update cargo

5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744 2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000

- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)

r+ `@ghost`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 3, 2022
Update cargo

5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744 2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000

- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)

r+ `@ghost`
@ehuss ehuss added this to the 1.67.0 milestone Dec 14, 2022
@epage epage deleted the atty branch November 28, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

atty dependency has security issue
5 participants