Avoid setting executable permissions on files we might not own (#5582)

## Summary

If we just created an entrypoint script, we can of course set the
permissions (we just created it). However, if we're copying from the
cache, we might _not_ own the file. In that case, if we need to change
the permissions (we shouldn't, since the script is likely already
executable -- we set the permissions when we unzip, but I guess they
could _not_ be properly set in the zip itself), we have to copy it.

Closes https://github.com/astral-sh/uv/issues/5581.
This commit is contained in:
Charlie Marsh 2024-07-30 08:32:52 -04:00 committed by GitHub
parent dfb4e5bbc8
commit 750b3a7c8c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 98 additions and 28 deletions

View file

@ -597,7 +597,7 @@ fn symlink_wheel_files(
// The `RECORD` file is modified during installation, so we copy it instead of symlinking. // The `RECORD` file is modified during installation, so we copy it instead of symlinking.
if path.ends_with("RECORD") { if path.ends_with("RECORD") {
fs::copy(path, &out_path)?; synchronized_copy(path, &out_path, locks)?;
count += 1; count += 1;
continue; continue;
} }

View file

@ -327,11 +327,14 @@ pub(crate) fn write_script_entrypoints(
// Make the launcher executable. // Make the launcher executable.
#[cfg(unix)] #[cfg(unix)]
{ {
use std::fs::Permissions;
use std::os::unix::fs::PermissionsExt; use std::os::unix::fs::PermissionsExt;
fs::set_permissions(
site_packages.join(entrypoint_relative), let path = site_packages.join(entrypoint_relative);
std::fs::Permissions::from_mode(0o755), let permissions = fs::metadata(&path)?.permissions();
)?; if permissions.mode() & 0o111 != 0o111 {
fs::set_permissions(path, Permissions::from_mode(permissions.mode() | 0o111))?;
}
} }
} }
} }
@ -534,20 +537,64 @@ fn install_script(
) )
})?; })?;
fs::remove_file(&path)?; fs::remove_file(&path)?;
// Make the script executable. We just created the file, so we can set permissions directly.
#[cfg(unix)]
{
use std::fs::Permissions;
use std::os::unix::fs::PermissionsExt;
let permissions = fs::metadata(&script_absolute)?.permissions();
if permissions.mode() & 0o111 != 0o111 {
fs::set_permissions(
script_absolute,
Permissions::from_mode(permissions.mode() | 0o111),
)?;
}
}
Some(size_and_encoded_hash) Some(size_and_encoded_hash)
} else { } else {
// reading and writing is slow especially for large binaries, so we move them instead // Reading and writing is slow (especially for large binaries), so we move them instead, if
// we can. This also retains the file permissions. We _can't_ move (and must copy) if the
// file permissions need to be changed, since we might not own the file.
drop(script); drop(script);
fs::rename(&path, &script_absolute)?;
#[cfg(unix)]
{
use std::fs::Permissions;
use std::os::unix::fs::PermissionsExt;
let permissions = fs::metadata(&path)?.permissions();
if permissions.mode() & 0o111 == 0o111 {
// If the permissions are already executable, we don't need to change them.
fs::rename(&path, &script_absolute)?;
} else {
// If we have to modify the permissions, copy the file, since we might not own it.
warn!(
"Copying script from {} to {} (permissions: {:o})",
path.simplified_display(),
script_absolute.simplified_display(),
permissions.mode()
);
uv_fs::copy_atomic_sync(&path, &script_absolute)?;
fs::set_permissions(
script_absolute,
Permissions::from_mode(permissions.mode() | 0o111),
)?;
}
}
#[cfg(not(unix))]
{
fs::rename(&path, &script_absolute)?;
}
None None
}; };
#[cfg(unix)]
{
use std::fs::Permissions;
use std::os::unix::fs::PermissionsExt;
fs::set_permissions(&script_absolute, Permissions::from_mode(0o755))?;
}
// Find the existing entry in the `RECORD`. // Find the existing entry in the `RECORD`.
let relative_to_site_packages = path let relative_to_site_packages = path

View file

@ -87,11 +87,13 @@ pub async fn unzip<R: tokio::io::AsyncRead + Unpin>(
let path = target.join(path); let path = target.join(path);
let permissions = fs_err::tokio::metadata(&path).await?.permissions(); let permissions = fs_err::tokio::metadata(&path).await?.permissions();
fs_err::tokio::set_permissions( if permissions.mode() & 0o111 != 0o111 {
&path, fs_err::tokio::set_permissions(
Permissions::from_mode(permissions.mode() | 0o111), &path,
) Permissions::from_mode(permissions.mode() | 0o111),
.await?; )
.await?;
}
} }
} }
} }
@ -137,11 +139,13 @@ async fn untar_in<R: tokio::io::AsyncRead + Unpin, P: AsRef<Path>>(
if has_any_executable_bit != 0 { if has_any_executable_bit != 0 {
if let Some(path) = crate::tar::unpacked_at(dst.as_ref(), &file.path()?) { if let Some(path) = crate::tar::unpacked_at(dst.as_ref(), &file.path()?) {
let permissions = fs_err::tokio::metadata(&path).await?.permissions(); let permissions = fs_err::tokio::metadata(&path).await?.permissions();
fs_err::tokio::set_permissions( if permissions.mode() & 0o111 != 0o111 {
&path, fs_err::tokio::set_permissions(
Permissions::from_mode(permissions.mode() | 0o111), &path,
) Permissions::from_mode(permissions.mode() | 0o111),
.await?; )
.await?;
}
} }
} }
} }

View file

@ -69,10 +69,12 @@ pub fn unzip<R: Send + std::io::Read + std::io::Seek + HasLength>(
let has_any_executable_bit = mode & 0o111; let has_any_executable_bit = mode & 0o111;
if has_any_executable_bit != 0 { if has_any_executable_bit != 0 {
let permissions = fs_err::metadata(&path)?.permissions(); let permissions = fs_err::metadata(&path)?.permissions();
fs_err::set_permissions( if permissions.mode() & 0o111 != 0o111 {
&path, fs_err::set_permissions(
Permissions::from_mode(permissions.mode() | 0o111), &path,
)?; Permissions::from_mode(permissions.mode() | 0o111),
)?;
}
} }
} }
} }

View file

@ -161,6 +161,23 @@ pub fn write_atomic_sync(path: impl AsRef<Path>, data: impl AsRef<[u8]>) -> std:
Ok(()) Ok(())
} }
/// Copy `from` to `to` atomically using a temporary file and atomic rename.
pub fn copy_atomic_sync(from: impl AsRef<Path>, to: impl AsRef<Path>) -> std::io::Result<()> {
let temp_file = tempfile_in(to.as_ref().parent().expect("Write path must have a parent"))?;
fs_err::copy(from.as_ref(), &temp_file)?;
temp_file.persist(&to).map_err(|err| {
std::io::Error::new(
std::io::ErrorKind::Other,
format!(
"Failed to persist temporary file to {}: {}",
to.user_display(),
err.error
),
)
})?;
Ok(())
}
/// Rename a file, retrying (on Windows) if it fails due to transient operating system errors. /// Rename a file, retrying (on Windows) if it fails due to transient operating system errors.
#[cfg(feature = "tokio")] #[cfg(feature = "tokio")]
pub async fn rename_with_retry( pub async fn rename_with_retry(