Added warning when using restricted names in Windows. by hbina · Pull Request #8136 · rust-lang/cargo

hbina · 2020-04-19T17:03:52Z

The implementation could have been better. I think there ought to be a way to only use the views into the OsString instead of creating a new one. I am new to Rust so any pointer will help ^^

rust-highfive · 2020-04-19T17:03:55Z

r? @Eh2406

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

ehuss · 2020-04-19T22:56:21Z

Thanks for the PR!

What I would do is extract this code into a new function fn is_windows_reserved_path(path: &Path) -> bool, and place it in the restricted_names module. You can then just call that with entry_path.

Also, I don't think there should be any message for non-windows platforms. These filenames aren't a problem for non-windows users.

You'll need to place this in the chain_err closure so that it only runs when extraction fails.

hbina · 2020-04-22T04:57:50Z

@ehuss something like this?

ehuss

Can you write a test for this? Use Package to publish a package with an invalid filename, and then try to use it as a dependency, and then run cargo build. Let me know if you have any questions about writing tests.

Also, it would be good to reuse is_windows_reserved_path in cargo_package::check_filename.

ehuss · 2020-04-22T17:34:45Z

src/cargo/util/restricted_names.rs

I think instead of fold, it might be simpler to use any. Also, it's probably more correct to check every path component. So this function could maybe be rewritten as:

path.iter() .filter_map(|component| component.to_str()) .any(|component| { let stem = component.split('.').next().unwrap(); is_windows_reserved(stem) })

ehuss · 2020-04-22T18:06:12Z

src/cargo/sources/registry/mod.rs

Would it be possible to structure this such that the error looks something roughly like this?

error: failed to download ... Caused by: failed to unpack package ... Caused by: failed to unpack entry at '{PATH}' Caused by: '{PATH}' appears to contain a reserved windows path, it cannot be extracted on Windows Caused by: ... More details from tar

So that the windows note is a separate item in the cause chain. You'll need to use let mut result = … to be able to conditionally chain the error, then result = result.chain_err(|| …); inside the condition.

Do you have an example of this kind of error chaining in the codebase?

No, but I think it would look something like this:

let mut result = entry.unpack_in(parent).map_err(anyhow::Error::from); if cfg!(windows) && restricted_names::is_windows_reserved_path(&entry_path) { result = result.chain_err(|| { format!( "`{}` appears to contain a reserved windows path, \ it cannot be extracted on Windows", entry_path.display() ) }); } result.chain_err(|| format!("failed to unpack entry at `{}`", entry_path.display()))?;

ehuss · 2020-04-22T18:06:39Z

src/cargo/sources/registry/mod.rs

I usually use deref to convert to a Path, so entry_path.as_path() can be &entry_path.

ehuss · 2020-04-22T18:07:16Z

src/cargo/sources/registry/mod.rs

This doesn't have any effect, since format! just evaluates to a string, it doesn't return from the closure.

hbina · 2020-04-23T03:08:29Z

Thanks for all the comments c: I will take a look at it later today.

hbina · 2020-04-23T20:03:22Z

So how do I write the test for this? I have this

#[cargo_test]
fn reserved_windows_name() {
    Package::new("aux", "1.0.0").publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
            [project]
            name = "foo"
            version = "0.0.1"
            authors = []
            license = "MIT"
            description = "foo"

            [dependencies]
            aux = "1.0.0"
        "#,
        )
        .file("src/main.rs", "extern crate aux;\nfn main() {  }")
        .build();
    p.cargo("package").run();
}

But this panics with in Windows.

failures:

---- package::reserved_windows_name stdout ----
thread 'package::reserved_windows_name' panicked at 'fs::create_dir_all(dst.parent().unwrap()) failed with The directory name is invalid. (os error 267)', crates\cargo-test-support\src\registry.rs:512:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    package::reserved_windows_name

test result: FAILED. 1802 passed; 1 failed; 3 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test testsuite'

ehuss · 2020-04-23T21:21:59Z

You'll want to create a package with a normal name (like bar), with a restricted file, like this:

Package::new("bar", "1.0.0")
    .file("src/lib.rs", "pub mod aux;")
    .file("src/aux.rs", "")
    .publish();

bors · 2020-04-24T19:09:07Z

☔ The latest upstream changes (presumably #8095) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2020-04-26T13:14:53Z

Looks great, thanks!
@bors r+

bors · 2020-04-26T13:14:54Z

📌 Commit 2c67111 has been approved by ehuss

bors · 2020-04-26T13:15:02Z

⌛ Testing commit 2c67111 with merge 4845c033736c43fbc4919cee24ac3884c9b7726c...

bors · 2020-04-26T13:51:40Z

💔 Test failed - checks-azure

ehuss · 2020-04-26T18:26:31Z

@bors retry
flaky test #8162

bors · 2020-04-26T18:26:39Z

⌛ Testing commit 2c67111 with merge 11f329d...

bors · 2020-04-26T18:48:36Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 11f329d to master...

Update cargo 11 commits in 8751eb3010d4cdb5329b5a6bd2b6d765c95b0dca..90931d9b31e8b854522fed00916504a3ac6d8619 2020-04-21 18:04:35 +0000 to 2020-04-28 01:56:59 +0000 - Use associated constants directly on primitive types instead of modules (rust-lang/cargo#8077) - Clear `RUSTDOCFLAGS` before running tests (rust-lang/cargo#8168) - Fix warning for `resolve` mismatch in workspace. (rust-lang/cargo#8169) - Fix flaky linking_interrupted test. (rust-lang/cargo#8162) - Fixed some unnecessary borrows and clones. (rust-lang/cargo#8146) - Added warning when using restricted names in Windows. (rust-lang/cargo#8136) - Add changelog about dylib uplift. (rust-lang/cargo#8161) - Mention that cargo_metadata can parse json messages (rust-lang/cargo#8158) - Re-enable rustc-info-cache test again (rust-lang/cargo#8155) - Updates to path source walking. (rust-lang/cargo#8095) - Bump to 0.46.0, update changelog (rust-lang/cargo#8153)

rust-highfive assigned Eh2406 Apr 19, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2020

ehuss reviewed Apr 22, 2020

View reviewed changes

hbina marked this pull request as ready for review April 24, 2020 19:04

ehuss mentioned this pull request Apr 26, 2020

Add checks for Windows reserved filenames in cargo install process #8160

Closed

Added warning when using restricted names in Windows.

2c67111

ehuss force-pushed the error_on_illegal_in_windows branch from bfaaa0b to 2c67111 Compare April 26, 2020 13:14

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 Apr 26, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 26, 2020

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 Apr 26, 2020

bors merged commit 11f329d into rust-lang:master Apr 26, 2020

ehuss mentioned this pull request Apr 26, 2020

Cargo should check for windows-reserved filenames when extracting a package. #8055

Closed

ehuss mentioned this pull request Apr 28, 2020

Update cargo rust-lang/rust#71642

Merged

ehuss added this to the 1.45.0 milestone Feb 6, 2022

Comments

Conversation

hbina commented Apr 19, 2020

Uh oh!

rust-highfive commented Apr 19, 2020

Uh oh!

ehuss commented Apr 19, 2020

Uh oh!

hbina commented Apr 22, 2020

Uh oh!

ehuss left a comment

Choose a reason for hiding this comment

Uh oh!

ehuss Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

ehuss Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

hbina Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

ehuss Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

ehuss Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

ehuss Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

hbina commented Apr 23, 2020

Uh oh!

hbina commented Apr 23, 2020

Uh oh!

ehuss commented Apr 23, 2020

Uh oh!

bors commented Apr 24, 2020

Uh oh!

ehuss commented Apr 26, 2020

Uh oh!

bors commented Apr 26, 2020

Uh oh!

bors commented Apr 26, 2020

Uh oh!

bors commented Apr 26, 2020

Uh oh!

ehuss commented Apr 26, 2020

Uh oh!

bors commented Apr 26, 2020

Uh oh!

bors commented Apr 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!