fix(turbopath): avoid incorrect lifetime in RelativeUnixPath::new

[chris-olszewski]

Description

I ended up hitting a panic using this function incorrectly, we can make it safer.

Explanation of Issue
Take the following code snippet:

 let absolute = AbsoluteSystemPath::new("/").unwrap();
 let foo = String::from("foo/bar/baz");
 let bar = RelativeUnixPath::new(foo).unwrap();
 let baz = absolute.join_unix_path(bar); // panics

It panics with the following error:

unsafe precondition(s) violated: ptr::copy_nonoverlapping requires that both pointer arguments are aligned and non-null and the specified memory ranges do not overlap

The issue is with the line let bar = RelativeUnixPath::new(foo); so lets take a look at the definition of RelativeUnixPath::new:

pub fn new<'a, P: AsRef<str> + 'a>(value: P) -> Result<&'a Self, PathError> {
    let path = value.as_ref();
    /* body omitted as it isn't relevant */
    Ok(unsafe { &*(path as *const str as *const Self) })
}

Lets break down the signature, new is generic over a lifetime 'a and a type P where P must implement AsRef<str> and be valid for at least the lifetime of 'a. new then returns a Result<&'a Self> aka a reference to a RelativeUnixPath valid for at least 'a.

It's important to clarify that P: AsRef<str> + 'a means P is valid for 'a, not the &str created by as_ref().

So if we pass a String object to RelativeUnixPath::new we end up with new<'a>(value: String) -> Result<&'a Self>, but what's the 'a? There's not any lifetime that's part of String!

If we look at the Static section of Rust By Example it contains the following remark:

It's important to understand this means that any owned data always passes a 'static lifetime bound

String is owned so it passes a bound such as T: 'static or in our case P: AsRef<str> + 'static.

This means that we can currently write a function like this:

fn foo(s: String) -> &'static RelativeUnixPath {
    RelativeUnixPath::new(s).unwrap()
}

This is clearly incorrect, foo takes ownership of s, but somehow returns a 'static reference even though s is consumed in the function and will be dropped at the end

Solution

The solution is that we make sure we make sure we take a type that's a reference! e.g.
fn new<'a, P: AsRef<str> + ?Sized>(value: &'a P) -> Result<&'a Self> this prevents us from passing an owned type into the constructor and smuggling in bad lifetimes to the codebase. (?Sized is just needed because Sized is an implicit trait bound in Rust and we're opting out of it. Since we're taking a reference, we don't need to know the size of the type see the Excotically Sized Types section of the Rustonomicon for more information)

Testing Instructions

The following will no longer compiles:

#[test]
fn test_pass_in_string() {
    let absolute = AbsoluteSystemPath::new("/").unwrap();
    let foo = String::from("foo/bar/baz");
    let bar = RelativeUnixPath::new(foo).unwrap();
    let baz = absolute.join_unix_path(bar); // panics
}

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-nonmonorepo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 7, 2024 9:12pm

8 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview		Nov 7, 2024 9:12pm

[arlyon]

Nice write-up! Looking at the other unsafe block in that file we should be ok if &self is OK and this PR fixes that.

While we are here, perhaps some proper SAFETY comments would be helpful for future people down the line. Don't have to do it everywhere but the paths lib is used by pretty much every turbo invocation

https://std-dev-guide.rust-lang.org/policy/safety-comments.html

We don't need the whole explainer some comment that the lifetime is attached to the lifetime of the owned object passed in so we can't end up with dangling pointers would be great.

Also is this code also prone to the same UB? I am not sure off the top of my head if PathBuf -> &Path is isomorphic to String -> &str or if they behave the same under APIT

turborepo/crates/turborepo-paths/src/anchored_system_path.rs

Line 48 in c03735d

pub(crate) unsafe fn new_unchecked<'a>(path: impl AsRef<Path> + 'a) -> &'a Self {

I guess that is why the fn is unsafe arguably but I wouldn't expect this kind of error from _unchecked, only that it wouldn't validate the structure of the data

[chris-olszewski]

Also is this code also prone to the same UB? I am not sure off the top of my head if PathBuf -> &Path is isomorphic to String -> &str or if they behave the same under APIT

Good catch!

[NicholasLYang]

Oops yeah that's probably my bad