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

Verify that src dir wasn't modified by build.rs when publishing #5584

Merged
merged 8 commits into from
May 29, 2018
10 changes: 10 additions & 0 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult
let id = SourceId::for_path(&dst)?;
let mut src = PathSource::new(&dst, &id, ws.config());
let new_pkg = src.root_package()?;
let pkg_fingerprint = src.fingerprint(&new_pkg)?;
let ws = Workspace::ephemeral(new_pkg, config, None, true)?;

ops::compile_ws(
Expand All @@ -352,6 +353,15 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult
Arc::new(DefaultExecutor),
)?;

// Check that build.rs didn't modify any files in the src directory.
let ws_fingerprint = src.fingerprint(ws.current()?)?;
if pkg_fingerprint != ws_fingerprint {
bail!(
"Source directory was modified by build.rs during cargo publish. \
Build scripts should not modify anything outside of OUT_DIR."
)
}

Ok(())
}

Expand Down
5 changes: 5 additions & 0 deletions src/doc/src/reference/build-scripts.md
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,11 @@ There’s a couple of points of note here:
output files should be located. It can use the process’ current working
directory to find where the input files should be located, but in this case we
don’t have any input files.
* In general, build scripts should not modify any files outside of `OUT_DIR`.
It may seem fine on the first blush, but it does cause problems when you use
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @matklad for formulating this already in the issue.

such crate as a dependency, because there's an *implicit* invariant that
sources in `.cargo/registry` should be immutable. `cargo` won't allow such
scripts when packaging.
* This script is relatively simple as it just writes out a small generated file.
One could imagine that other more fanciful operations could take place such as
generating a Rust module from a C header file or another language definition,
Expand Down
35 changes: 35 additions & 0 deletions tests/testsuite/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1416,3 +1416,38 @@ fn lock_file_and_workspace() {
fname.ends_with("Cargo.lock")
}));
}

#[test]
fn do_not_package_if_src_was_modified() {
let p = project("foo")
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.0.1"
authors = []
"#)
.file("src/main.rs", r#"
fn main() { println!("hello"); }
"#)
.file("build.rs", r#"
use std::fs::File;
use std::io::Write;

fn main() {
let mut file = File::create("src/generated.txt").expect("failed to create file");
file.write_all(b"Hello, world of generated files.").expect("failed to write");
}
"#)
.build();

assert_that(
p.cargo("package"),
execs().with_status(101),
);

assert_that(
p.cargo("package")
Copy link
Member

Choose a reason for hiding this comment

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

This can be more compactly written as p.cargo("package --no-verify"), the testing infra gained ability to split arguments automatically fairly recently.

.arg("--no-verify"),
execs().with_status(0),
);
}