Use Astral-maintained `tokio-tar` fork by charliermarsh · Pull Request #11174 · astral-sh/uv

charliermarsh · 2025-02-03T01:02:43Z

Summary

I shipped one security fix here along with several significant performance improvements for large TAR files:

I also PR'd the security fix to edera-dev (edera-dev/tokio-tar#4).

charliermarsh · 2025-02-03T01:34:38Z

Converting to draft while I checkout failing debug assertions.

charliermarsh · 2025-02-03T01:48:07Z

crates/uv-extract/src/stream.rs

+
+    // Delay any directory entries until the end, to ensure that directory permissions do not
+    // interfere with descendant extraction.
+    let mut directories = Vec::new();


This is related to astral-sh/tokio-tar#2. I forgot that we don't actually use the crate's unpack method, we unpack ourselves (since we need to set permissions in a different way).

charliermarsh · 2025-02-03T01:48:34Z

crates/uv-extract/src/stream.rs

                let mode = file.header().mode()?;
                let has_any_executable_bit = mode & 0o111;
                if has_any_executable_bit != 0 {
-                    if let Some(path) = crate::tar::unpacked_at(dst, &file.path()?) {


We can get rid of this whole vendored unpacked_at implementation. It turns out the crate now returns the target path. (That's not a change I made, it must've been made in one of the forks.)

konstin · 2025-02-03T12:38:20Z

crates/uv-extract/src/stream.rs

+    // Delay any directory entries until the end, to ensure that directory permissions do not
+    // interfere with descendant extraction.


Is this a concern for us? We usually create directories from archives without preserving permissions (at least in the zip path we should).

Also maybe this is me being unfamiliar with the codebase but this is so... bizarre. How does this unpacking a file before the dir that contains it make sense? Is the idea that we create_dir_all any file path anyway, so the dir entries only create empty dirs and set perms on dirs that have files?

For zips we always create the parent of a file, as a zip can contain directory entries, but sometimes there are no directory entries, just files.

uv/crates/uv-extract/src/stream.rs

Lines 66 to 76 in 4d3809c

// Either create the directory or write the file to disk.

if is_dir {

if directories.insert(path.clone()) {

fs_err::tokio::create_dir_all(path).await?;

}

} else {

if let Some(parent) = path.parent() {

if directories.insert(parent.to_path_buf()) {

fs_err::tokio::create_dir_all(parent).await?;

}

}

Is the idea that we create_dir_all any file path anyway, so the dir entries only create empty dirs and set perms on dirs that have files?

Yes. When we create a file, we create the directories required for it. This happens within the async-tar crate. The reason we have to do it "manually" here is because we have our own unpack that replicates the async-tar logic but applies different permissions.

(I will look at why the tar crate introduced this before merging.)

It kind of seems like a bug that we aren't preserving directory permissions in the zip case?

personally, i'd prefer only preserving the executable bit and otherwise using the default umask; i'd find it odd if the permissions on the build host should determine the permissions in the deployment target.

Yeah. Probably requires more invasive changes to the tar crate though.

I'd like to review what pip does here before making other changes (since this is just preserving our status quo).

Nevermind, pip explicitly only preserves the executable bit. I'll make the same change for tar in a separate PR.

https://github.com/pypa/pip/blob/3898741e29b7279e7bffe044ecfbe20f6a438b1e/src/pip/_internal/utils/unpacking.py#L88-L100

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.25` -> `0.5.27` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.27`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0527) [Compare Source](astral-sh/uv@0.5.26...0.5.27) ##### Enhancements - Avoid setting permissions during tar extraction ([#11191](astral-sh/uv#11191)) - Remove warnings for missing lower bounds ([#11195](astral-sh/uv#11195)) - Update PubGrub to set-based outdated priority tracking ([#11169](astral-sh/uv#11169)) - Improve error messages for `uv pip install` with `--extra` or `--all-extras` and invalid sources ([#11193](astral-sh/uv#11193)) - Sign Docker images using GitHub attestations ([#8685](astral-sh/uv#8685)) ##### Preview features - Don't expand self-referential extras in the build backend ([#11142](astral-sh/uv#11142)) ##### Performance - Filter discovered Python executables by source before querying ([#11143](astral-sh/uv#11143)) - Optimize exclusion computation for markers ([#11158](astral-sh/uv#11158)) - Use Astral-maintained `tokio-tar` fork ([#11174](astral-sh/uv#11174)) - Remove unneeded `.clone()` ([#11127](astral-sh/uv#11127)) ##### Bug fixes - Fix relative paths in bytecode compilation ([#11177](astral-sh/uv#11177)) - Percent-decode URLs in canonical comparisons ([#11088](astral-sh/uv#11088)) - Respect concurrency limits in parallel index fetch ([#11182](astral-sh/uv#11182)) - Use wire JSON schema for conflict items ([#11196](astral-sh/uv#11196)) - Use explicit `_GLibCVersion` tuple in uv-python crate ([#11122](astral-sh/uv#11122)) ##### Documentation - Add Git SHA locking behavior to docs ([#11125](astral-sh/uv#11125)) - Add best-practice flags to `pip install` example in troubleshooting guide ([#11194](astral-sh/uv#11194)) - Set `VIRTUAL_ENV` in Jupyter kernels ([#11155](astral-sh/uv#11155)) - Add instructions for deactivating an environment ([#11200](astral-sh/uv#11200)) ### [`v0.5.26`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0526) [Compare Source](astral-sh/uv@0.5.25...0.5.26) ##### Enhancements - Add support for `uvx python` ([#11076](astral-sh/uv#11076)) - Allow `--no-dev --invert` in `uv tree` ([#11068](astral-sh/uv#11068)) - Update `uv python install --reinstall` to reinstall all previous versions ([#11072](astral-sh/uv#11072)) - Consistently write log messages with capitalized first word ([#11111](astral-sh/uv#11111)) - Suggest `--build-backend` when `--backend` is passed to `uv init` ([#10958](astral-sh/uv#10958)) - Improve retry trace message ([#11108](astral-sh/uv#11108)) ##### Performance - Remove unnecessary UTF-8 conversion in hash parsing ([#11110](astral-sh/uv#11110)) ##### Bug fixes - Ignore non-hash fragments in HTML API responses ([#11107](astral-sh/uv#11107)) - Avoid resolving symbolic links when querying Python interpreters ([#11083](astral-sh/uv#11083)) - Avoid sharing state between universal and non-universal resolves ([#11051](astral-sh/uv#11051)) - Error when `--script` is passing a non-PEP 723 script ([#11118](astral-sh/uv#11118)) - Make metadata deserialization failures non-fatal in the cache ([#11105](astral-sh/uv#11105)) - Mark metadata as dynamic when reading from built wheel cache ([#11046](astral-sh/uv#11046)) - Propagate credentials for `<index>/simple` to `<index>/...` endpoints ([#11074](astral-sh/uv#11074)) - Fix conflicting extra bug during `uv sync` ([#11075](astral-sh/uv#11075)) ##### Documentation - Add PyTorch XPU instructions to the PyTorch guide ([#11109](astral-sh/uv#11109)) - Add docs for signal handling ([#11041](astral-sh/uv#11041)) - Explain build frontend vs. build backend ([#11094](astral-sh/uv#11094)) - Fix formatting of `RUST_LOG` documentation ([#10053](astral-sh/uv#10053)) - Fix typo in `--no-deps` description ([#11073](astral-sh/uv#11073)) - Reflow CLI documentation comments ([#11040](astral-sh/uv#11040)) - Shorten "Using existing Python versions" nav item so it fits on one line ([#11077](astral-sh/uv#11077)) - Some minor touch-ups to the Python install guide ([#11116](astral-sh/uv#11116)) - Update Dependabot tracking issue link ([#11054](astral-sh/uv#11054)) - Update documentation for running in a container ([#11052](astral-sh/uv#11052)) - Upgrade PyTorch version in documentation ([#11114](astral-sh/uv#11114)) - Use `sys_platform` in lieu of `platform_system` in PyTorch docs ([#11113](astral-sh/uv#11113)) - Use positive (rather than negative) markers in PyTorch examples ([#11112](astral-sh/uv#11112)) - Fix unnecessary backslashes in brackets ([#11059](astral-sh/uv#11059)) - Suggest setting copy link mode in GitLab integration guide ([#11067](astral-sh/uv#11067)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

charliermarsh added performance Potential performance improvement security labels Feb 3, 2025

charliermarsh requested review from Gankra, konstin and zanieb February 3, 2025 01:02

charliermarsh temporarily deployed to uv-test-publish February 3, 2025 01:09 — with GitHub Actions Inactive

charliermarsh marked this pull request as draft February 3, 2025 01:34

charliermarsh force-pushed the charlie/mark branch 2 times, most recently from c5eaf84 to 49fe355 Compare February 3, 2025 01:45

charliermarsh commented Feb 3, 2025

View reviewed changes

charliermarsh temporarily deployed to uv-test-publish February 3, 2025 01:48 — with GitHub Actions Inactive

charliermarsh force-pushed the charlie/mark branch from 49fe355 to 9069e02 Compare February 3, 2025 01:55

charliermarsh marked this pull request as ready for review February 3, 2025 01:56

charliermarsh temporarily deployed to uv-test-publish February 3, 2025 01:58 — with GitHub Actions Inactive

konstin approved these changes Feb 3, 2025

View reviewed changes

Gankra approved these changes Feb 3, 2025

View reviewed changes

Use Astral-maintained tokio-tar fork

0092e56

charliermarsh force-pushed the charlie/mark branch from 9069e02 to 0092e56 Compare February 3, 2025 17:43

charliermarsh temporarily deployed to uv-test-publish February 3, 2025 17:45 — with GitHub Actions Inactive

charliermarsh enabled auto-merge (squash) February 3, 2025 17:47

charliermarsh merged commit 7b43baf into main Feb 3, 2025
90 of 91 checks passed

charliermarsh deleted the charlie/mark branch February 3, 2025 17:51

BrewTestBot mentioned this pull request Feb 4, 2025

uv 0.5.27 Homebrew/homebrew-core#206465

Merged

		// Delay any directory entries until the end, to ensure that directory permissions do not
		// interfere with descendant extraction.

	// Either create the directory or write the file to disk.
	if is_dir {
	if directories.insert(path.clone()) {
	fs_err::tokio::create_dir_all(path).await?;
	}
	} else {
	if let Some(parent) = path.parent() {
	if directories.insert(parent.to_path_buf()) {
	fs_err::tokio::create_dir_all(parent).await?;
	}
	}

Comments

Conversation

charliermarsh commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

charliermarsh commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliermarsh Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charliermarsh commented Feb 3, 2025 •

edited

Loading

charliermarsh Feb 3, 2025 •

edited

Loading