Auto merge of #5584 - boxdot:master, r=matklad

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

Fixes issue #5073.

Implemented during RustFest Paris `impl days`.

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.last_modified_file(&new_pkg)?;
     let ws = Workspace::ephemeral(new_pkg, config, None, true)?;
 
     ops::compile_ws(
@@ -352,6 +353,19 @@ 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.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. \
+             Modified file: {}\n\n\
+             To proceed despite this, pass the `--no-verify` flag.",
+            path.display()
+        )
+    }
+
     Ok(())
 }
 

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<String> {
-        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()))
     }
 }

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,

tests/testsuite/package.rs

@@ -1416,3 +1416,47 @@ 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)
+               .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(
+        p.cargo("package --no-verify"),
+        execs().with_status(0),
+    );
+}