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

Try to better handle restricted crate names. #7959

Merged
merged 3 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ tar = { version = "0.4.26", default-features = false }
tempfile = "3.0"
termcolor = "1.0"
toml = "0.5.3"
unicode-xid = "0.2.0"
url = "2.0"
walkdir = "2.2"
clap = "2.31.2"
Expand Down
6 changes: 0 additions & 6 deletions src/cargo/core/compiler/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,6 @@ pub struct Layout {
_lock: FileLock,
}

pub fn is_bad_artifact_name(name: &str) -> bool {
["deps", "examples", "build", "incremental"]
.iter()
.any(|&reserved| reserved == name)
}

impl Layout {
/// Calculate the paths for build output, lock the build directory, and return as a Layout.
///
Expand Down
1 change: 0 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ pub use self::custom_build::{BuildOutput, BuildScriptOutputs, BuildScripts};
pub use self::job::Freshness;
use self::job::{Job, Work};
use self::job_queue::{JobQueue, JobState};
pub use self::layout::is_bad_artifact_name;
use self::output_depinfo::output_depinfo;
use self::unit_dependencies::UnitDep;
pub use crate::core::compiler::unit::{Unit, UnitInterner};
Expand Down
97 changes: 67 additions & 30 deletions src/cargo/ops/cargo_new.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::core::{compiler, Workspace};
use crate::core::{Shell, Workspace};
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo};
use crate::util::{paths, validate_package_name, Config};
use crate::util::{paths, restricted_names, Config};
use git2::Config as GitConfig;
use git2::Repository as GitRepository;
use serde::de;
Expand Down Expand Up @@ -155,41 +155,71 @@ fn get_name<'a>(path: &'a Path, opts: &'a NewOptions) -> CargoResult<&'a str> {
})
}

fn check_name(name: &str, opts: &NewOptions) -> CargoResult<()> {
// If --name is already used to override, no point in suggesting it
// again as a fix.
let name_help = match opts.name {
Some(_) => "",
None => "\nuse --name to override crate name",
};
fn check_name(name: &str, name_help: &str, has_bin: bool, shell: &mut Shell) -> CargoResult<()> {
restricted_names::validate_package_name(name, "crate name", name_help)?;

// Ban keywords + test list found at
// https://doc.rust-lang.org/reference/keywords.html
let blacklist = [
"abstract", "alignof", "as", "become", "box", "break", "const", "continue", "crate", "do",
"else", "enum", "extern", "false", "final", "fn", "for", "if", "impl", "in", "let", "loop",
"macro", "match", "mod", "move", "mut", "offsetof", "override", "priv", "proc", "pub",
"pure", "ref", "return", "self", "sizeof", "static", "struct", "super", "test", "trait",
"true", "type", "typeof", "unsafe", "unsized", "use", "virtual", "where", "while", "yield",
];
if blacklist.contains(&name) || (opts.kind.is_bin() && compiler::is_bad_artifact_name(name)) {
if restricted_names::is_keyword(name) {
anyhow::bail!(
"The name `{}` cannot be used as a crate name{}",
"the name `{}` cannot be used as a crate name, it is a Rust keyword{}",
name,
name_help
)
);
}

if let Some(ref c) = name.chars().next() {
if c.is_digit(10) {
if restricted_names::is_conflicting_artifact_name(name) {
if has_bin {
anyhow::bail!(
"Package names starting with a digit cannot be used as a crate name{}",
"the name `{}` cannot be used as a crate name, \
it conflicts with cargo's build directory names{}",
name,
name_help
)
);
} else {
shell.warn(format!(
"the name `{}` will not support binary \
executables with that name, \
it conflicts with cargo's build directory names",
name
))?;
}
}
if name == "test" {
anyhow::bail!(
"the name `test` cannot be used as a crate name, \
it conflicts with Rust's built-in test library{}",
name_help
);
}
if ["core", "std", "alloc", "proc_macro", "proc-macro"].contains(&name) {
shell.warn(format!(
"the name `{}` is part of Rust's standard library\n\
It is recommended to use a different name to avoid problems.",
name
))?;
}
if restricted_names::is_windows_reserved(name) {
if cfg!(windows) {
anyhow::bail!(
"cannot use name `{}`, it is a reserved Windows filename{}",
name,
name_help
);
} else {
shell.warn(format!(
"the name `{}` is a reserved Windows filename\n\
This package will not work on Windows platforms.",
name
))?;
}
}
if restricted_names::is_non_ascii_name(name) {
shell.warn(format!(
"the name `{}` contains non-ASCII characters\n\
Support for non-ASCII crate names is experimental and only valid \
on the nightly toolchain.",
name
))?;
}

validate_package_name(name, "crate name", name_help)?;
Ok(())
}

Expand Down Expand Up @@ -337,7 +367,7 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
}

let name = get_name(path, opts)?;
check_name(name, opts)?;
check_name(name, "", opts.kind.is_bin(), &mut config.shell())?;

let mkopts = MkOptions {
version_control: opts.version_control,
Expand Down Expand Up @@ -372,7 +402,6 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
}

let name = get_name(path, opts)?;
check_name(name, opts)?;

let mut src_paths_types = vec![];

Expand All @@ -385,6 +414,14 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
// Maybe when doing `cargo init --bin` inside a library package stub,
// user may mean "initialize for library, but also add binary target"
}
let has_bin = src_paths_types.iter().any(|x| x.bin);
// If --name is already used to override, no point in suggesting it
// again as a fix.
let name_help = match opts.name {
Some(_) => "",
None => "\nuse --name to override crate name",
};
check_name(name, name_help, has_bin, &mut config.shell())?;

let mut version_control = opts.version_control;

Expand Down Expand Up @@ -426,7 +463,7 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
version_control,
path,
name,
bin: src_paths_types.iter().any(|x| x.bin),
bin: has_bin,
source_files: src_paths_types,
edition: opts.edition.as_ref().map(|s| &**s),
registry: opts.registry.as_ref().map(|s| &**s),
Expand Down
28 changes: 24 additions & 4 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ use log::debug;
use tar::{Archive, Builder, EntryType, Header};

use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor};
use crate::core::{Feature, Verbosity, Workspace};
use crate::core::{Feature, Shell, Verbosity, Workspace};
use crate::core::{Package, PackageId, PackageSet, Resolve, Source, SourceId};
use crate::ops;
use crate::sources::PathSource;
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::paths;
use crate::util::toml::TomlManifest;
use crate::util::{self, Config, FileLock};
use crate::util::{self, restricted_names, Config, FileLock};

pub struct PackageOpts<'cfg> {
pub config: &'cfg Config,
Expand Down Expand Up @@ -142,7 +142,7 @@ fn build_ar_list(
let root = pkg.root();
for src_file in src_files {
let rel_path = src_file.strip_prefix(&root)?.to_path_buf();
check_filename(&rel_path)?;
check_filename(&rel_path, &mut ws.config().shell())?;
let rel_str = rel_path
.to_str()
.ok_or_else(|| {
Expand Down Expand Up @@ -804,7 +804,7 @@ fn report_hash_difference(orig: &HashMap<PathBuf, u64>, after: &HashMap<PathBuf,
//
// To help out in situations like this, issue about weird filenames when
// packaging as a "heads up" that something may not work on other platforms.
fn check_filename(file: &Path) -> CargoResult<()> {
fn check_filename(file: &Path, shell: &mut Shell) -> CargoResult<()> {
let name = match file.file_name() {
Some(name) => name,
None => return Ok(()),
Expand All @@ -825,5 +825,25 @@ fn check_filename(file: &Path) -> CargoResult<()> {
file.display()
)
}
let mut check_windows = |name| -> CargoResult<()> {
if restricted_names::is_windows_reserved(name) {
shell.warn(format!(
"file {} is a reserved Windows filename, \
it will not work on Windows platforms",
file.display()
))?;
}
Ok(())
};
for component in file.iter() {
if let Some(component) = component.to_str() {
check_windows(component)?;
}
}
if file.extension().is_some() {
if let Some(stem) = file.file_stem().and_then(|s| s.to_str()) {
check_windows(stem)?;
}
}
Ok(())
}
18 changes: 2 additions & 16 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub use self::paths::{dylib_path_envvar, normalize_path};
pub use self::process_builder::{process, ProcessBuilder};
pub use self::progress::{Progress, ProgressStyle};
pub use self::read2::read2;
pub use self::restricted_names::validate_package_name;
pub use self::rustc::Rustc;
pub use self::sha256::Sha256;
pub use self::to_semver::ToSemver;
Expand Down Expand Up @@ -51,6 +52,7 @@ pub mod process_builder;
pub mod profile;
mod progress;
mod read2;
pub mod restricted_names;
pub mod rustc;
mod sha256;
pub mod to_semver;
Expand All @@ -68,22 +70,6 @@ pub fn elapsed(duration: Duration) -> String {
}
}

/// Check the base requirements for a package name.
///
/// This can be used for other things than package names, to enforce some
/// level of sanity. Note that package names have other restrictions
/// elsewhere. `cargo new` has a few restrictions, such as checking for
/// reserved names. crates.io has even more restrictions.
pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<()> {
if let Some(ch) = name
.chars()
.find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-')
{
anyhow::bail!("Invalid character `{}` in {}: `{}`{}", ch, what, name, help);
}
Ok(())
}

/// Whether or not this running in a Continuous Integration environment.
pub fn is_ci() -> bool {
std::env::var("CI").is_ok() || std::env::var("TF_BUILD").is_ok()
Expand Down
83 changes: 83 additions & 0 deletions src/cargo/util/restricted_names.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//! Helpers for validating and checking names like package and crate names.

use crate::util::CargoResult;
use anyhow::bail;

/// Returns `true` if the name contains non-ASCII characters.
pub fn is_non_ascii_name(name: &str) -> bool {
name.chars().any(|ch| ch > '\x7f')
}

/// A Rust keyword.
pub fn is_keyword(name: &str) -> bool {
// See https://doc.rust-lang.org/reference/keywords.html
[
"Self", "abstract", "as", "async", "await", "become", "box", "break", "const", "continue",
"crate", "do", "dyn", "else", "enum", "extern", "false", "final", "fn", "for", "if",
"impl", "in", "let", "loop", "macro", "match", "mod", "move", "mut", "override", "priv",
"pub", "ref", "return", "self", "static", "struct", "super", "trait", "true", "try",
"type", "typeof", "unsafe", "unsized", "use", "virtual", "where", "while", "yield",
]
.contains(&name)
}

/// These names cannot be used on Windows, even with an extension.
pub fn is_windows_reserved(name: &str) -> bool {
[
"con", "prn", "aux", "nul", "com1", "com2", "com3", "com4", "com5", "com6", "com7", "com8",
"com9", "lpt1", "lpt2", "lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9",
Copy link
Member

Choose a reason for hiding this comment

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

So just to clarify, lpt9.foobarbazhello is an invalid filename on Windows?

All of these are invalid as the filestem of a file anywhere on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct on the extension.

As for "anywhere", that's a bit of a stretch. It is possible to create these files, like with cygwin tools which use different APIs, but those files end up causing problems with the rest of the Windows ecosystem.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, thanks for clarifying! I agree yeah that I've seen these "work" in some contexts but it gets really weird when only some contexts have them work and others don't.

]
.contains(&name.to_ascii_lowercase().as_str())
}

/// An artifact with this name will conflict with one of Cargo's build directories.
pub fn is_conflicting_artifact_name(name: &str) -> bool {
["deps", "examples", "build", "incremental"].contains(&name)
}

/// Check the base requirements for a package name.
///
/// This can be used for other things than package names, to enforce some
/// level of sanity. Note that package names have other restrictions
/// elsewhere. `cargo new` has a few restrictions, such as checking for
/// reserved names. crates.io has even more restrictions.
pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<()> {
let mut chars = name.chars();
if let Some(ch) = chars.next() {
if ch.is_digit(10) {
// A specific error for a potentially common case.
bail!(
"the name `{}` cannot be used as a {}, \
the name cannot start with a digit{}",
name,
what,
help
);
}
if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_') {
bail!(
"invalid character `{}` in {}: `{}`, \
the first character must be a Unicode XID start character \
(most letters or `_`){}",
ch,
what,
name,
help
);
}
}
for ch in chars {
if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-') {
bail!(
"invalid character `{}` in {}: `{}`, \
characters must be Unicode XID characters \
(numbers, `-`, `_`, or most letters){}",
ch,
what,
name,
help
);
}
}
Ok(())
}
Loading