Added warning when using restricted names in Windows.

[hbina]

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]

r? @Eh2406

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

[ehuss]

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]

@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]

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]

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.

[hbina]

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

[ehuss]

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]

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

[ehuss]

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

[hbina]

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

[hbina]

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]

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]

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

[ehuss]

Looks great, thanks!
@bors r+

[bors]

📌 Commit 2c67111 has been approved by ehuss

[bors]

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

[bors]

💔 Test failed - checks-azure

[ehuss]

@bors retry
flaky test #8162

[bors]

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

[bors]

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