From e09449c2c36b4eab52fa45762b4451e2b4ab6a03 Mon Sep 17 00:00:00 2001
From: Chris Olszewski <chris.olszewski@vercel.com>
Date: Thu, 7 Nov 2024 16:42:52 -0500
Subject: [PATCH] fix(turbopath): avoid incorrect lifetime in
 RelativeUnixPath::new (#9400)

### 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](https://doc.rust-lang.org/rust-by-example/scope/lifetime/static_lifetime.html#trait-bound)
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](https://doc.rust-lang.org/nomicon/exotic-sizes.html#dynamically-sized-types-dsts)
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
}
```
---
 crates/turborepo-paths/src/anchored_system_path.rs | 4 ++--
 crates/turborepo-paths/src/relative_unix_path.rs   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/crates/turborepo-paths/src/anchored_system_path.rs b/crates/turborepo-paths/src/anchored_system_path.rs
index f0d6863963e44..4f9e379ce23bd 100644
--- a/crates/turborepo-paths/src/anchored_system_path.rs
+++ b/crates/turborepo-paths/src/anchored_system_path.rs
@@ -45,12 +45,12 @@ impl AsRef<Path> for AnchoredSystemPath {
 const EMPTY: &str = "";
 
 impl AnchoredSystemPath {
-    pub(crate) unsafe fn new_unchecked<'a>(path: impl AsRef<Path> + 'a) -> &'a Self {
+    pub(crate) unsafe fn new_unchecked(path: &(impl AsRef<Path> + ?Sized)) -> &Self {
         let path = path.as_ref();
         unsafe { &*(path as *const Path as *const Self) }
     }
 
-    pub fn new<'a>(path: impl AsRef<str> + 'a) -> Result<&'a Self, PathError> {
+    pub fn new(path: &(impl AsRef<str> + ?Sized)) -> Result<&Self, PathError> {
         let path_str = path.as_ref();
         let path = Path::new(path_str);
         if path.is_absolute() {
diff --git a/crates/turborepo-paths/src/relative_unix_path.rs b/crates/turborepo-paths/src/relative_unix_path.rs
index 9cfd6027dffc7..abf2a29b35bee 100644
--- a/crates/turborepo-paths/src/relative_unix_path.rs
+++ b/crates/turborepo-paths/src/relative_unix_path.rs
@@ -18,7 +18,7 @@ impl Display for RelativeUnixPath {
 }
 
 impl RelativeUnixPath {
-    pub fn new<'a, P: AsRef<str> + 'a>(value: P) -> Result<&'a Self, PathError> {
+    pub fn new<P: AsRef<str> + ?Sized>(value: &P) -> Result<&Self, PathError> {
         let path = value.as_ref();
         if path.starts_with('/') {
             return Err(PathError::NotRelative(path.to_string()));