Skip to content

Commit

Permalink
Fix 'LocalRequest::clone()' soundness issue.
Browse files Browse the repository at this point in the history
The existing implementation of 'LocalRequest::clone()' mistakenly copied
the internal 'Request' pointer from the existing 'LocalRequest' to the
cloned 'LocalRequest'. This resulted in an aliased '*mut Request'
pointer, a clear soundness issue. The fix in this commit is to clone the
internal 'Request', replacing the internal pointer with the newly cloned
'Request' when producing the cloned 'LocalRequest'. A fix that removes
all 'unsafe' code should be explored.

Fixes #1312.
  • Loading branch information
SergioBenitez committed May 27, 2020
1 parent ca4d157 commit b8f9011
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 4 deletions.
24 changes: 20 additions & 4 deletions core/lib/src/local/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -476,19 +476,35 @@ impl fmt::Debug for LocalResponse<'_> {

impl<'c> Clone for LocalRequest<'c> {
fn clone(&self) -> LocalRequest<'c> {
// Don't alias the existing `Request`. See #1312.
let mut request = Rc::new(self.inner().clone());
let ptr = Rc::get_mut(&mut request).unwrap() as *mut Request<'_>;

LocalRequest {
ptr, request,
client: self.client,
ptr: self.ptr,
request: self.request.clone(),
data: self.data.clone(),
uri: self.uri.clone()
}
}
}

// #[cfg(test)]
#[cfg(test)]
mod tests {
// Someday...
use crate::Request;
use crate::local::Client;

#[test]
fn clone_unique_ptr() {
let client = Client::new(crate::ignite()).unwrap();
let r1 = client.get("/");
let r2 = r1.clone();

assert_ne!(
r1.inner() as *const Request<'_>,
r2.inner() as *const Request<'_>
);
}

// #[test]
// #[compile_fail]
Expand Down
33 changes: 33 additions & 0 deletions core/lib/tests/unsound-local-request-1312.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
use rocket::http::Header;
use rocket::local::Client;

#[test]
fn test_local_request_clone_soundness() {
let client = Client::new(rocket::ignite()).unwrap();

// creates two LocalRequest instances that shouldn't share the same req
let r1 = client.get("/").header(Header::new("key", "val1"));
let mut r2 = r1.clone();

// save the iterator, which internally holds a slice
let mut iter = r1.inner().headers().get("key");

// insert headers to force header map reallocation.
for i in 0..100 {
r2.add_header(Header::new(i.to_string(), i.to_string()));
}

// Replace the original key/val.
r2.add_header(Header::new("key", "val2"));

// Heap massage: so we've got crud to print.
let _: Vec<usize> = vec![0, 0xcafebabe, 31337, 0];

// Ensure we're good.
let s = iter.next().unwrap();
println!("{}", s);

// And that we've got the right data.
assert_eq!(r1.inner().headers().get("key").collect::<Vec<_>>(), vec!["val1"]);
assert_eq!(r2.inner().headers().get("key").collect::<Vec<_>>(), vec!["val1", "val2"]);
}

0 comments on commit b8f9011

Please sign in to comment.