Fixes `cargo package` buffering entire contents into memory #7946

hbina · 2020-02-27T21:47:29Z

Added a helper function for util::hex::hash_64 that uses streams
the content instead of reading through the entire content in one
go.

Edit: Should I add test cases for this?

Edit2: Per #7543

Added a helper function for util::hex::hash_64 that uses streams the content instead of reading through the entire content in one go.

rust-highfive · 2020-02-27T21:47:32Z

r? @Eh2406

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

Eh2406 · 2020-02-27T21:55:05Z

src/cargo/util/hex.rs

+        if n == 0 {
+            break;
+        }
+        hasher.write(&buf);


comparing it to

cargo/src/cargo/util/sha256.rs

Lines 20 to 29 in ab2b2c0

pub fn update_file(&mut self, mut file: &File) -> io::Result<&mut Sha256> {

let mut buf = [0; 64 * 1024];

loop {

let n = file.read(&mut buf)?;

if n == 0 {

break Ok(self);

}

self.update(&buf[..n]);

}

}

I'd have expected hasher.write(&buf[..n]);

Oof, sorry about that.

Eh2406 · 2020-02-27T21:58:52Z

The red CI is unrelated (cc 54242f50f8582c2d891f8552c561e244c000c5d8d2a4d8bfb6696114a784708d if I feal like investigating.)

Over all this look good with one nit. But I don't know the context well. So,
@bors r? @ehuss

hbina · 2020-02-27T22:12:51Z

I am getting this error

hbina@akarin:~/Documents/test_cargo$ cargo publish --dry-run
    Updating crates.io index
warning: manifest has no description, license, license-file, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging test_cargo_7543 v0.1.0 (/home/hbina/Documents/test_cargo)
   Verifying test_cargo_7543 v0.1.0 (/home/hbina/Documents/test_cargo)
   Compiling test_cargo_7543 v0.1.0 (/home/hbina/Documents/test_cargo/target/package/test_cargo_7543-0.1.0)
    Finished dev [unoptimized + debuginfo] target(s) in 1.56s
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.
Added: /home/hbina/Documents/test_cargo/target/package/test_cargo_7543-0.1.0/tests/generated/large.txt

To proceed despite this, pass the `--no-verify` flag.

So I am not sure if it actually works.

Edit:...Or maybe this is proofs that it works?

FauxFaux · 2020-02-27T22:28:14Z

src/cargo/util/hex.rs

+    let mut hasher = SipHasher::new_with_keys(0, 0);
+    let mut buf = [0; 64 * 1024];
+    loop {
+        let n = file.read(&mut buf).unwrap();


The original code returns errors, instead of panicing.

Yeah, I have changed it but now all the tests are failing..

hbina · 2020-02-28T18:22:20Z

Thanks for reviewing!

alexcrichton · 2020-02-28T18:35:23Z

@bors: r+

bors · 2020-02-28T18:35:24Z

📌 Commit 0ac7aee has been approved by alexcrichton

bors · 2020-02-28T18:35:31Z

⌛ Testing commit 0ac7aee with merge 443e276...

bors · 2020-02-28T18:52:39Z

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 443e276 to master...

Update cargo, clippy Closes #69601 ## cargo 16 commits in e57bd02999c9f40d52116e0beca7d1dccb0643de..bda50510d1daf6e9c53ad6ccf603da6e0fa8103f 2020-02-21 20:20:10 +0000 to 2020-03-02 18:05:34 +0000 - Fix rare failure in collision_export test. (rust-lang/cargo#7956) - Ignore broken Cargo.toml in git sources (rust-lang/cargo#7947) - Add more fingerprint mtime debug logging. (rust-lang/cargo#7952) - Fix plugin tests for latest nightly. (rust-lang/cargo#7955) - Simplified usage code of SipHasher (rust-lang/cargo#7945) - Add a special case for git config discovery inside tests (rust-lang/cargo#7944) - Fixes issue rust-lang/cargo#7543 (rust-lang/cargo#7946) - Filter out cfgs which should not be used during build (rust-lang/cargo#7943) - Provide extra context on a query failure. (rust-lang/cargo#7934) - Try to clarify `cargo metadata`'s relationship with the workspace. (rust-lang/cargo#7927) - Update libgit2 dependency (rust-lang/cargo#7939) - Fix link in comment (rust-lang/cargo#7936) - Enable `cargo doc --open` tests on macos. (rust-lang/cargo#7932) - build-std: remove sysroot probe (rust-lang/cargo#7931) - Try to clarify how feature flags work on the "current" package. (rust-lang/cargo#7928) - Add extra details in the new feature resolver doc comment. (rust-lang/cargo#7918) ## clippy 6 commits in fc5d0cc..8b7f7e6 2020-02-24 05:58:17 +0000 to 2020-03-02 20:00:31 +0000 - Rustup to #69469 (rust-lang/rust-clippy#5254) - Some rustups (rust-lang/rust-clippy#5247) - Update git2 to 0.12 (rust-lang/rust-clippy#5232) - Rustup to #61812 (rust-lang/rust-clippy#5231) - Add lint to improve floating-point expressions (rust-lang/rust-clippy#4897) - Do not run deploy action on other repos (rust-lang/rust-clippy#5222)

Fixes issue rust-lang#7543

bdec8c2

Added a helper function for util::hex::hash_64 that uses streams the content instead of reading through the entire content in one go.

rust-highfive assigned Eh2406 Feb 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2020

Eh2406 reviewed Feb 27, 2020

View reviewed changes

rust-highfive assigned ehuss and unassigned Eh2406 Feb 27, 2020

Fix erronous error with call to write

c3997d6

FauxFaux reviewed Feb 27, 2020

View reviewed changes

hash_u64_file now returns an Error instead of panicking.

0ac7aee

alexcrichton approved these changes Feb 28, 2020

View reviewed changes

hbina marked this pull request as ready for review February 28, 2020 18:15

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2020

bors merged commit 443e276 into rust-lang:master Feb 28, 2020

ehuss mentioned this pull request Mar 2, 2020

Update cargo, clippy rust-lang/rust#69627

Merged

ehuss changed the title ~~Fixes issue #7543~~ Fixes cargo package buffering entire contents into memory Mar 4, 2020

ehuss mentioned this pull request Mar 4, 2020

cargo publish "memory allocation of .. bytes failed" with large, ignored test assets #7543

Closed

hbina deleted the add_stream_hashu64 branch June 8, 2020 17:32

ehuss added this to the 1.43.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes `cargo package` buffering entire contents into memory #7946

Fixes `cargo package` buffering entire contents into memory #7946

hbina commented Feb 27, 2020 •

edited

Loading

rust-highfive commented Feb 27, 2020

Eh2406 Feb 27, 2020

hbina Feb 27, 2020

Eh2406 commented Feb 27, 2020

hbina commented Feb 27, 2020 •

edited

Loading

FauxFaux Feb 27, 2020

hbina Feb 27, 2020

hbina commented Feb 28, 2020

alexcrichton commented Feb 28, 2020

bors commented Feb 28, 2020

bors commented Feb 28, 2020

bors commented Feb 28, 2020

	pub fn update_file(&mut self, mut file: &File) -> io::Result<&mut Sha256> {
	let mut buf = [0; 64 * 1024];
	loop {
	let n = file.read(&mut buf)?;
	if n == 0 {
	break Ok(self);
	}
	self.update(&buf[..n]);
	}
	}

Fixes cargo package buffering entire contents into memory #7946

Fixes cargo package buffering entire contents into memory #7946

Conversation

hbina commented Feb 27, 2020 • edited Loading

rust-highfive commented Feb 27, 2020

Eh2406 Feb 27, 2020

Choose a reason for hiding this comment

hbina Feb 27, 2020

Choose a reason for hiding this comment

Eh2406 commented Feb 27, 2020

hbina commented Feb 27, 2020 • edited Loading

FauxFaux Feb 27, 2020

Choose a reason for hiding this comment

hbina Feb 27, 2020

Choose a reason for hiding this comment

hbina commented Feb 28, 2020

alexcrichton commented Feb 28, 2020

bors commented Feb 28, 2020

bors commented Feb 28, 2020

bors commented Feb 28, 2020

Fixes `cargo package` buffering entire contents into memory #7946

Fixes `cargo package` buffering entire contents into memory #7946

hbina commented Feb 27, 2020 •

edited

Loading

hbina commented Feb 27, 2020 •

edited

Loading