-
Notifications
You must be signed in to change notification settings - Fork 760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid calling normalize_path
with relative paths that extend beyond the current directory
#3013
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,8 +84,16 @@ pub fn normalize_url_path(path: &str) -> Cow<'_, str> { | |
/// Normalize a path, removing things like `.` and `..`. | ||
/// | ||
/// Source: <https://github.com/rust-lang/cargo/blob/b48c41aedbd69ee3990d62a0e2006edbb506a480/crates/cargo-util/src/paths.rs#L76C1-L109C2> | ||
pub fn normalize_path(path: impl AsRef<Path>) -> PathBuf { | ||
let mut components = path.as_ref().components().peekable(); | ||
/// | ||
/// CAUTION: Assumes that the path is already absolute. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Can we panic if it isn't absolute? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. |
||
/// | ||
/// CAUTION: This does not resolve symlinks (unlike | ||
/// [`std::fs::canonicalize`]). This may cause incorrect or surprising | ||
/// behavior at times. This should be used carefully. Unfortunately, | ||
/// [`std::fs::canonicalize`] can be hard to use correctly, since it can often | ||
/// fail, or on Windows returns annoying device paths. | ||
pub fn normalize_path(path: &Path) -> PathBuf { | ||
let mut components = path.components().peekable(); | ||
let mut ret = if let Some(c @ Component::Prefix(..)) = components.peek().copied() { | ||
components.next(); | ||
PathBuf::from(c.as_os_str()) | ||
|
@@ -235,7 +243,7 @@ mod tests { | |
use super::*; | ||
|
||
#[test] | ||
fn normalize() { | ||
fn normalize_url() { | ||
if cfg!(windows) { | ||
assert_eq!( | ||
normalize_url_path("/C:/Users/ferris/wheel-0.42.0.tar.gz"), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -488,16 +488,15 @@ impl InterpreterInfo { | |
/// unless the Python executable changes, so we use the executable's last modified | ||
/// time as a cache key. | ||
pub(crate) fn query_cached(executable: &Path, cache: &Cache) -> Result<Self, Error> { | ||
let executable_bytes = executable.as_os_str().as_encoded_bytes(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. \cc @BurntSushi - I changed this to just hash the path directly. Not sure if it's equivalent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to grok why you need the hashes to be the same here. I'm not understanding that park. Like, even if this generates a different hash, that seems okay, it just means you'll get a cache miss? Or is there something deeper here that I'm missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They don't need to be the same! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was mostly a Chesterton's Fence of trying to understand why we did this in the first place. |
||
let canonical = uv_fs::canonicalize_executable(executable)?; | ||
let modified = Timestamp::from_path(&canonical)?; | ||
|
||
let cache_entry = cache.entry( | ||
CacheBucket::Interpreter, | ||
"", | ||
format!("{}.msgpack", digest(&executable_bytes)), | ||
format!("{}.msgpack", digest(&canonical)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @konstin - I think it's okay to cache under the canonical path here rather than the executable path... since we already use the canonical path for the timestamp. It could cause issues with executables that are symlinks (that's a common source of bugs here), but I think it's still fine since we're not using the canonical path to query the executable, which is what tends to cause issues. |
||
); | ||
|
||
let modified = Timestamp::from_path(uv_fs::canonicalize_executable(executable)?)?; | ||
|
||
// Read from the cache. | ||
if cache | ||
.freshness(&cache_entry, None) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we panic if it isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.