Skip to content

Commit

Permalink
fix(turbopath): avoid incorrect lifetime in RelativeUnixPath::new (#9400
Browse files Browse the repository at this point in the history
)

### 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
}
```
  • Loading branch information
chris-olszewski authored Nov 7, 2024
1 parent b65ff15 commit e09449c
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
4 changes: 2 additions & 2 deletions crates/turborepo-paths/src/anchored_system_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-paths/src/relative_unix_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down

0 comments on commit e09449c

Please sign in to comment.