From ca9aa3dae88f58576169c10e4b1ed925b9e07725 Mon Sep 17 00:00:00 2001 From: boxdot Date: Mon, 28 May 2018 12:06:03 +0200 Subject: [PATCH 1/8] Verify that src dir was not modified by build.rs during publish. Co-authored-by: Gabriel Feron --- src/cargo/ops/cargo_package.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index f9de8286824..037d53ba916 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -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( @@ -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(()) } From 8896fddef80eb276ba98fd772d11bdbcf1a2f8e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Mon, 28 May 2018 12:05:46 +0200 Subject: [PATCH 2/8] Add test to check that publish fails when src directory was changed. Co-authored-by: boxdot --- tests/testsuite/publish.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 0eba5a8ddac..ac7e5c7e47e 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -872,3 +872,36 @@ fn block_publish_no_registry() { ), ); } + +#[test] +fn do_not_publish_if_src_was_modified() { + publish::setup(); + + 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("publish") + .arg("--index") + .arg(publish::registry().to_string()), + execs().with_status(101), + ); +} \ No newline at end of file From ba48ff427388b8d68115fb12723318dd431c1acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Mon, 28 May 2018 13:59:35 +0200 Subject: [PATCH 3/8] Add cargo --no-verify test when checking that src dir was not modified while publishing --- tests/testsuite/package.rs | 35 +++++++++++++++++++++++++++++++++++ tests/testsuite/publish.rs | 33 --------------------------------- 2 files changed, 35 insertions(+), 33 deletions(-) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 235a91f3d3d..08aad2f0359 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -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") + .arg("--no-verify"), + execs().with_status(0), + ); +} diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index ac7e5c7e47e..0eba5a8ddac 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -872,36 +872,3 @@ fn block_publish_no_registry() { ), ); } - -#[test] -fn do_not_publish_if_src_was_modified() { - publish::setup(); - - 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("publish") - .arg("--index") - .arg(publish::registry().to_string()), - execs().with_status(101), - ); -} \ No newline at end of file From 285eb4d7b9569ff9d31dd6b0ee1ccf51ef5d4f24 Mon Sep 17 00:00:00 2001 From: boxdot Date: Mon, 28 May 2018 14:11:52 +0200 Subject: [PATCH 4/8] Document that build scripts should not modify files outside OUT_DIR. Co-authored-by: Gabriel Feron --- src/doc/src/reference/build-scripts.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/doc/src/reference/build-scripts.md b/src/doc/src/reference/build-scripts.md index beb762869ca..fbf71a6d6f7 100644 --- a/src/doc/src/reference/build-scripts.md +++ b/src/doc/src/reference/build-scripts.md @@ -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 + 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, From 9c1124fbcf7c8392bfc32341bd427b709046abc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Mon, 28 May 2018 14:34:47 +0200 Subject: [PATCH 5/8] Provide more context when src files changed while packaging Co-authored-by: boxdot --- src/cargo/ops/cargo_package.rs | 9 ++++--- src/cargo/sources/path.rs | 44 ++++++++++++++++++---------------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 037d53ba916..84115a6da87 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -330,7 +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 pkg_fingerprint = src.last_modified_file(&new_pkg)?; let ws = Workspace::ephemeral(new_pkg, config, None, true)?; ops::compile_ws( @@ -354,11 +354,14 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult )?; // Check that build.rs didn't modify any files in the src directory. - let ws_fingerprint = src.fingerprint(ws.current()?)?; + let ws_fingerprint = src.last_modified_file(ws.current()?)?; if pkg_fingerprint != ws_fingerprint { + let (_, path) = ws_fingerprint; bail!( "Source directory was modified by build.rs during cargo publish. \ - Build scripts should not modify anything outside of OUT_DIR." + Build scripts should not modify anything outside of OUT_DIR. \ + Modified file: {}", + path.display() ) } diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index c7a0fdf75c4..f058b25700f 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -467,6 +467,29 @@ impl<'cfg> PathSource<'cfg> { } Ok(()) } + + pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { + if !self.updated { + return Err(internal("BUG: source was not updated")); + } + + let mut max = FileTime::zero(); + let mut max_path = PathBuf::new(); + for file in self.list_files(pkg)? { + // An fs::stat error here is either because path is a + // broken symlink, a permissions error, or a race + // condition where this path was rm'ed - either way, + // we can ignore the error and treat the path's mtime + // as 0. + let mtime = paths::mtime(&file).unwrap_or(FileTime::zero()); + if mtime > max { + max = mtime; + max_path = file; + } + } + trace!("last modified file {}: {}", self.path.display(), max); + Ok((max, max_path)) + } } impl<'cfg> Debug for PathSource<'cfg> { @@ -516,26 +539,7 @@ impl<'cfg> Source for PathSource<'cfg> { } fn fingerprint(&self, pkg: &Package) -> CargoResult { - if !self.updated { - return Err(internal("BUG: source was not updated")); - } - - let mut max = FileTime::zero(); - let mut max_path = PathBuf::from(""); - for file in self.list_files(pkg)? { - // An fs::stat error here is either because path is a - // broken symlink, a permissions error, or a race - // condition where this path was rm'ed - either way, - // we can ignore the error and treat the path's mtime - // as 0. - let mtime = paths::mtime(&file).unwrap_or(FileTime::zero()); - warn!("{} {}", mtime, file.display()); - if mtime > max { - max = mtime; - max_path = file; - } - } - trace!("fingerprint {}: {}", self.path.display(), max); + let (max, max_path) = self.last_modified_file(pkg)?; Ok(format!("{} ({})", max, max_path.display())) } } From e31f3c3ca966500bf1e7c21444dcffd04faed0e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Mon, 28 May 2018 14:43:17 +0200 Subject: [PATCH 6/8] Use auto split argument in test --- tests/testsuite/package.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 08aad2f0359..beb650c7233 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1446,8 +1446,7 @@ fn do_not_package_if_src_was_modified() { ); assert_that( - p.cargo("package") - .arg("--no-verify"), + p.cargo("package --no-verify"), execs().with_status(0), ); } From 53480ac7c710ec1c276213e85b859943e17704a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Tue, 29 May 2018 11:05:34 +0200 Subject: [PATCH 7/8] Add more instructions to error message --- src/cargo/ops/cargo_package.rs | 5 +++-- tests/testsuite/package.rs | 12 +++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 84115a6da87..2b55b19d110 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -360,8 +360,9 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult bail!( "Source directory was modified by build.rs during cargo publish. \ Build scripts should not modify anything outside of OUT_DIR. \ - Modified file: {}", - path.display() + Modified file: {}\n\n\ + To proceed despite this, pass the `--no-verify` flag.", + path.display() ) } diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index beb650c7233..fde31b76c72 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1442,7 +1442,17 @@ fn do_not_package_if_src_was_modified() { assert_that( p.cargo("package"), - execs().with_status(101), + execs().with_status(101) + .with_stderr_contains( + "\ +error: failed to verify package tarball + +Caused by: + Source directory was modified by build.rs during cargo publish. \ +Build scripts should not modify anything outside of OUT_DIR. Modified file: [..]src/generated.txt + +To proceed despite this, pass the `--no-verify` flag.", + ), ); assert_that( From 987ce8751ffd774b15733cc8402d66934b573aae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Tue, 29 May 2018 11:38:27 +0200 Subject: [PATCH 8/8] Fix test to work on Windows --- tests/testsuite/package.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index fde31b76c72..01b7f1a371a 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1449,7 +1449,7 @@ error: failed to verify package tarball Caused by: Source directory was modified by build.rs during cargo publish. \ -Build scripts should not modify anything outside of OUT_DIR. Modified file: [..]src/generated.txt +Build scripts should not modify anything outside of OUT_DIR. Modified file: [..]src[/]generated.txt To proceed despite this, pass the `--no-verify` flag.", ),