Fix 'LocalRequest::clone()' soundness issue.

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.
This commit is contained in:
Sergio Benitez 2020-05-27 01:09:12 -07:00
parent ca4d1572d4
commit b8f9011c04
2 changed files with 53 additions and 4 deletions

View File

@ -476,19 +476,35 @@ impl fmt::Debug for LocalResponse<'_> {
impl<'c> Clone for LocalRequest<'c> { impl<'c> Clone for LocalRequest<'c> {
fn clone(&self) -> 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 { LocalRequest {
ptr, request,
client: self.client, client: self.client,
ptr: self.ptr,
request: self.request.clone(),
data: self.data.clone(), data: self.data.clone(),
uri: self.uri.clone() uri: self.uri.clone()
} }
} }
} }
// #[cfg(test)] #[cfg(test)]
mod tests { 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] // #[test]
// #[compile_fail] // #[compile_fail]

View File

@ -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"]);
}