Rework 'local_cache!' so it returns unique refs.

Fixes #1987.

core/lib/src/data/from_data.rs

@@ -214,7 +214,7 @@ impl<'r> FromData<'r> for Capped<&'r str> {
 
     async fn from_data(req: &'r Request<'_>, data: Data<'r>) -> Outcome<'r, Self> {
         let capped = try_outcome!(<Capped<String>>::from_data(req, data).await);
-        let string = capped.map(|s| local_cache!(req, s).as_str());
+        let string = capped.map(|s| local_cache!(req, s));
         Success(string)
     }
 }
@@ -252,7 +252,7 @@ impl<'r> FromData<'r> for Capped<&'r [u8]> {
 
     async fn from_data(req: &'r Request<'_>, data: Data<'r>) -> Outcome<'r, Self> {
         let capped = try_outcome!(<Capped<Vec<u8>>>::from_data(req, data).await);
-        let raw = capped.map(|b| local_cache!(req, b).as_slice());
+        let raw = capped.map(|b| local_cache!(req, b));
         Success(raw)
     }
 }

core/lib/src/form/buffer.rs

@@ -0,0 +1,114 @@
+use std::ops::{Index, RangeFrom, RangeTo};
+use std::cell::UnsafeCell;
+
+use parking_lot::{RawMutex, lock_api::RawMutex as _};
+
+mod private {
+    /// Sealed trait for types that can be shared in a `SharedStack`.
+    ///
+    /// The type of values passed to
+    /// [`local_cache`](crate::request::local_cache) must implement this trait.
+    /// Since this trait is sealed, the types implementing this trait are known
+    /// and finite: `String` and `Vec<T> for all T: Sync + Send + 'static`.
+    // UNSAFE: Needs to have a stable address when deref'd.
+    pub unsafe trait Shareable: std::ops::Deref + Sync + Send + 'static {
+        /// The current length of the owned shareable.
+        fn len(&self) -> usize;
+    }
+
+    unsafe impl Shareable for String {
+        fn len(&self) -> usize { self.len() }
+    }
+
+    unsafe impl<T: Send + Sync + 'static> Shareable for Vec<T> {
+        fn len(&self) -> usize { self.len() }
+    }
+}
+
+pub use private::Shareable;
+
+/// A stack of strings (chars of bytes) that can be shared between threads while
+/// remaining internally mutable and while allowing references into the stack to
+/// persist across mutations.
+pub struct SharedStack<T: Shareable> {
+    stack: UnsafeCell<Vec<T>>,
+    mutex: RawMutex,
+}
+
+impl<T: Shareable> SharedStack<T>
+    where T::Target: Index<RangeFrom<usize>, Output = T::Target> +
+                     Index<RangeTo<usize>, Output = T::Target>
+{
+    /// Creates a new stack.
+    pub fn new() -> Self {
+        SharedStack {
+            stack: UnsafeCell::new(vec![]),
+            mutex: RawMutex::INIT,
+        }
+    }
+
+    /// Pushes the string `S` onto the stack. Returns a reference of the string
+    /// in the stack.
+    pub(crate) fn push<S: Into<T>>(&self, string: S) -> &T::Target {
+        // SAFETY:
+        //   * Aliasing: We retrieve a mutable reference to the last slot (via
+        //     `push()`) and then return said reference as immutable; these
+        //     occur in serial, so they don't alias. This method accesses a
+        //     unique slot each call: the last slot, subsequently replaced by
+        //     `push()` each next call. No other method accesses the internal
+        //     buffer directly. Thus, the outstanding reference to the last slot
+        //     is never accessed again mutably, preserving aliasing guarantees.
+        //   * Liveness: The returned reference is to a `String`; we must ensure
+        //     that the `String` is never dropped while `self` lives. This is
+        //     guaranteed by returning a reference with the same lifetime as
+        //     `self`, so `self` can't be dropped while the string is live, and
+        //     by never removing elements from the internal `Vec` thus not
+        //     dropping `String` itself: `push()` is the only mutating operation
+        //     called on `Vec`, which preserves all previous elements; the
+        //     stability of `String` itself means that the returned address
+        //     remains valid even after internal realloc of `Vec`.
+        //   * Thread-Safety: Parallel calls to `push_one` without exclusion
+        //     would result in a race to `vec.push()`; `RawMutex` ensures that
+        //     this doesn't occur.
+        unsafe {
+            self.mutex.lock();
+            let vec: &mut Vec<T> = &mut *self.stack.get();
+            vec.push(string.into());
+            let last = vec.last().expect("push() => non-empty");
+            self.mutex.unlock();
+            last
+        }
+    }
+
+    /// Just like `push` but `string` must already be the owned `T`.
+    pub fn push_owned(&self, string: T) -> &T::Target {
+        self.push(string)
+    }
+
+    /// Pushes the string `S` onto the stack which is assumed to internally
+    /// contain two strings with the first string being of length `n`. Returns
+    /// references to the two strings on the stack.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `string.len() < len`.
+    pub(crate) fn push_split<S: Into<T>>(&self, string: S, n: usize) -> (&T::Target, &T::Target) {
+        let buffered = self.push(string);
+        let a = &buffered[..n];
+        let b = &buffered[n..];
+        (a, b)
+    }
+
+    /// Pushes the strings `a` and `b` onto the stack without allocating for
+    /// both strings. Returns references to the two strings on the stack.
+    pub(crate) fn push_two<'a, V>(&'a self, a: V, b: V) -> (&'a T::Target, &'a T::Target)
+        where T: From<V> + Extend<V>,
+    {
+        let mut value = T::from(a);
+        let split_len = value.len();
+        value.extend(Some(b));
+        self.push_split(value, split_len)
+    }
+}
+
+unsafe impl<T: Shareable> Sync for SharedStack<T> {}

core/lib/src/form/form.rs

@@ -4,7 +4,7 @@ use crate::Request;
 use crate::outcome::try_outcome;
 use crate::data::{Data, FromData, Outcome};
 use crate::http::{RawStr, ext::IntoOwned};
-use crate::form::parser::{Parser, RawStrParser, Buffer};
+use crate::form::{SharedStack, parser::{Parser, RawStrParser}};
 use crate::form::prelude::*;
 
 /// A data guard for [`FromForm`] types.
@@ -244,7 +244,7 @@ impl<T: for<'a> FromForm<'a> + 'static> Form<T> {
     /// assert_eq!(pet.wags, true);
     /// ```
     pub fn parse_encoded(string: &RawStr) -> Result<'static, T> {
-        let buffer = Buffer::new();
+        let buffer = SharedStack::new();
         let mut ctxt = T::init(Options::Lenient);
         for field in RawStrParser::new(&buffer, string) {
             T::push_value(&mut ctxt, field)

core/lib/src/form/mod.rs

@@ -368,6 +368,7 @@ mod context;
 mod strict;
 mod lenient;
 mod parser;
+mod buffer;
 pub mod validate;
 pub mod name;
 pub mod error;
@@ -384,6 +385,9 @@ pub use rocket_codegen::{FromForm, FromFormField};
 #[doc(inline)]
 pub use self::error::{Errors, Error};
 
+#[doc(hidden)]
+pub use self::buffer::{SharedStack, Shareable};
+
 pub use field::*;
 pub use options::*;
 pub use from_form_field::*;

core/lib/src/form/parser.rs

@@ -1,32 +1,24 @@
-use std::cell::UnsafeCell;
-
 use multer::Multipart;
-use parking_lot::{RawMutex, lock_api::RawMutex as _};
 use either::Either;
 
-use crate::request::{Request, local_cache};
+use crate::request::{Request, local_cache_once};
 use crate::data::{Data, Limits, Outcome};
-use crate::form::prelude::*;
+use crate::form::{SharedStack, prelude::*};
 use crate::http::RawStr;
 
 type Result<'r, T> = std::result::Result<T, Error<'r>>;
 
 type Field<'r, 'i> = Either<ValueField<'r>, DataField<'r, 'i>>;
 
-pub struct Buffer {
-    strings: UnsafeCell<Vec<String>>,
-    mutex: RawMutex,
-}
-
 pub struct MultipartParser<'r, 'i> {
     request: &'r Request<'i>,
-    buffer: &'r Buffer,
+    buffer: &'r SharedStack<String>,
     source: Multipart<'r>,
     done: bool,
 }
 
 pub struct RawStrParser<'r> {
-    buffer: &'r Buffer,
+    buffer: &'r SharedStack<String>,
     source: &'r RawStr,
 }
 
@@ -60,8 +52,8 @@ impl<'r, 'i> Parser<'r, 'i> {
         }
 
         Ok(Parser::RawStr(RawStrParser {
-            buffer: local_cache!(req, Buffer::new()),
+            buffer: local_cache_once!(req, SharedStack::new()),
-            source: RawStr::new(local_cache!(req, string.into_inner())),
+            source: RawStr::new(local_cache_once!(req, string.into_inner())),
         }))
     }
 
@@ -77,7 +69,7 @@ impl<'r, 'i> Parser<'r, 'i> {
 
         Ok(Parser::Multipart(MultipartParser {
             request: req,
-            buffer: local_cache!(req, Buffer::new()),
+            buffer: local_cache_once!(req, SharedStack::new()),
             source: Multipart::with_reader(data.open(form_limit), boundary),
             done: false,
         }))
@@ -92,7 +84,7 @@ impl<'r, 'i> Parser<'r, 'i> {
 }
 
 impl<'r> RawStrParser<'r> {
-    pub fn new(buffer: &'r Buffer, source: &'r RawStr) -> Self {
+    pub fn new(buffer: &'r SharedStack<String>, source: &'r RawStr) -> Self {
         RawStrParser { buffer, source }
     }
 }
@@ -119,8 +111,8 @@ impl<'r> Iterator for RawStrParser<'r> {
         trace_!("url-encoded field: {:?}", (name, value));
         let name_val = match (name.url_decode_lossy(), value.url_decode_lossy()) {
             (Borrowed(name), Borrowed(val)) => (name, val),
-            (Borrowed(name), Owned(v)) => (name, self.buffer.push_one(v)),
+            (Borrowed(name), Owned(v)) => (name, self.buffer.push(v)),
-            (Owned(name), Borrowed(val)) => (self.buffer.push_one(name), val),
+            (Owned(name), Borrowed(val)) => (self.buffer.push(name), val),
             (Owned(mut name), Owned(val)) => {
                 let len = name.len();
                 name.push_str(&val);
@@ -138,14 +130,14 @@ mod raw_str_parse_tests {
 
     #[test]
     fn test_skips_empty() {
-        let buffer = super::Buffer::new();
+        let buffer = super::SharedStack::new();
         let fields: Vec<_> = super::RawStrParser::new(&buffer, "a&b=c&&&c".into()).collect();
         assert_eq!(fields, &[Field::parse("a"), Field::parse("b=c"), Field::parse("c")]);
     }
 
     #[test]
     fn test_decodes() {
-        let buffer = super::Buffer::new();
+        let buffer = super::SharedStack::new();
         let fields: Vec<_> = super::RawStrParser::new(&buffer, "a+b=c%20d&%26".into()).collect();
         assert_eq!(fields, &[Field::parse("a b=c d"), Field::parse("&")]);
     }
@@ -174,8 +166,8 @@ impl<'r, 'i> MultipartParser<'r, 'i> {
         let field = if let Some(content_type) = content_type {
             let (name, file_name) = match (field.name(), field.file_name()) {
                 (None, None) => ("", None),
-                (None, Some(file_name)) => ("", Some(self.buffer.push_one(file_name))),
+                (None, Some(file_name)) => ("", Some(self.buffer.push(file_name))),
-                (Some(name), None) => (self.buffer.push_one(name), None),
+                (Some(name), None) => (self.buffer.push(name), None),
                 (Some(a), Some(b)) => {
                     let (field_name, file_name) = self.buffer.push_two(a, b);
                     (field_name, Some(file_name))
@@ -207,60 +199,3 @@ impl<'r, 'i> MultipartParser<'r, 'i> {
         Some(Ok(field))
     }
 }
-
-impl Buffer {
-    pub fn new() -> Self {
-        Buffer {
-            strings: UnsafeCell::new(vec![]),
-            mutex: RawMutex::INIT,
-        }
-    }
-
-    pub fn push_one<S: Into<String>>(&self, string: S) -> &str {
-        // SAFETY:
-        //   * Aliasing: We retrieve a mutable reference to the last slot (via
-        //     `push()`) and then return said reference as immutable; these
-        //     occur in serial, so they don't alias. This method accesses a
-        //     unique slot each call: the last slot, subsequently replaced by
-        //     `push()` each next call. No other method accesses the internal
-        //     buffer directly. Thus, the outstanding reference to the last slot
-        //     is never accessed again mutably, preserving aliasing guarantees.
-        //   * Liveness: The returned reference is to a `String`; we must ensure
-        //     that the `String` is never dropped while `self` lives. This is
-        //     guaranteed by returning a reference with the same lifetime as
-        //     `self`, so `self` can't be dropped while the string is live, and
-        //     by never removing elements from the internal `Vec` thus not
-        //     dropping `String` itself: `push()` is the only mutating operation
-        //     called on `Vec`, which preserves all previous elements; the
-        //     stability of `String` itself means that the returned address
-        //     remains valid even after internal realloc of `Vec`.
-        //   * Thread-Safety: Parallel calls to `push_one` without exclusion
-        //     would result in a race to `vec.push()`; `RawMutex` ensures that
-        //     this doesn't occur.
-        unsafe {
-            self.mutex.lock();
-            let vec: &mut Vec<String> = &mut *self.strings.get();
-            vec.push(string.into());
-            let last = vec.last().expect("push() => non-empty");
-            self.mutex.unlock();
-            last
-        }
-    }
-
-    pub fn push_split(&self, string: String, len: usize) -> (&str, &str) {
-        let buffered = self.push_one(string);
-        let a = &buffered[..len];
-        let b = &buffered[len..];
-        (a, b)
-    }
-
-    pub fn push_two<'a>(&'a self, a: &str, b: &str) -> (&'a str, &'a str) {
-        let mut buffer = String::new();
-        buffer.push_str(a);
-        buffer.push_str(b);
-
-        self.push_split(buffer, a.len())
-    }
-}
-
-unsafe impl Sync for Buffer {}

core/lib/src/request/mod.rs

@@ -17,13 +17,72 @@ pub use crate::response::flash::FlashMessage;
 pub(crate) use self::request::ConnectionMeta;
 
 crate::export! {
-    /// Store and immediately retrieve a value `$v` in `$request`'s local cache
+    /// Store and immediately retrieve a vector-like value `$v` (`String` or
-    /// using a locally generated anonymous type to avoid type conflicts.
+    /// `Vec<T>`) in `$request`'s local cache using a locally generated
+    /// anonymous type to avoid type conflicts.
+    ///
+    /// Unlike `local_cache_once`, this macro's generated code _always_ returns
+    /// a unique reference to request-local cache.
+    ///
+    /// # Note
+    ///
+    /// The value `$v` must be of type `String` or `Vec<T>`, that is, a type
+    /// that implements the sealed trait [`Shareable`](crate::form::Shareable)bb).
     ///
     /// # Example
     ///
     /// ```rust
-    /// use rocket::request::local_cache;
+    /// use rocket::request::{local_cache, local_cache_once};
+    /// # let c = rocket::local::blocking::Client::debug_with(vec![]).unwrap();
+    /// # let request = c.get("/");
+    ///
+    /// // The first store into local cache for a given type wins.
+    /// for i in 0..4 {
+    ///     assert_eq!(request.local_cache(|| i.to_string()), "0");
+    /// }
+    ///
+    /// // This shows that we cannot cache different values of the same type; we
+    /// // _must_ use a proxy type. To avoid the need to write these manually, use
+    /// // `local_cache!`, which generates one of the fly.
+    /// for i in 0..4 {
+    ///     assert_eq!(local_cache!(request, i.to_string()), i.to_string());
+    /// }
+    ///
+    /// // Note that while `local_cache_once!` generates a new type for the
+    /// // _macro_ invocation, that type is the same per run-time invocation, so
+    /// // all "calls" to `local_cache_once!` on the same line return the same
+    /// // reference for a given request.
+    /// for i in 1..4 {
+    ///     // Note that this is `1`, so _not_ the `String` from line 4.
+    ///     assert_eq!(local_cache_once!(request, i.to_string()), "1");
+    /// }
+    /// ```
+    macro_rules! local_cache {
+        ($request:expr, $v:expr $(,)?) => ({
+            struct Local<T: $crate::form::Shareable>($crate::form::SharedStack<T>);
+            let stack = $request.local_cache(|| Local($crate::form::SharedStack::new()));
+            stack.0.push_owned($v)
+        })
+    }
+}
+
+crate::export! {
+    /// Store and immediately retrieve a value `$v` in `$request`'s local cache
+    /// using a locally generated anonymous type to avoid type conflicts.
+    ///
+    /// The code generated by this macro is expected to be invoked at-most once
+    /// per-request. This is because while `local_cache_once!` generates a new
+    /// type for the _macro_ invocation, that type is the same per run-time
+    /// invocation. Thus, for a given request, a `local_cache_once!` invocation
+    /// always returns the same reference.
+    ///
+    /// To get a unique request-local reference to string-like values, use
+    /// [`local_cache!`] instead.
+    ///
+    /// # Example
+    ///
+    /// ```rust
+    /// use rocket::request::local_cache_once;
     /// # let c = rocket::local::blocking::Client::debug_with(vec![]).unwrap();
     /// # let request = c.get("/");
     ///
@@ -33,13 +92,19 @@ crate::export! {
     /// // The following returns the cached, previously stored value for the type.
     /// assert_eq!(request.local_cache(|| String::from("goodbye")), "hello");
     ///
-    /// // This shows that we cannot cache different values of the same type; we
+    /// // This shows that we cannot cache different values of the same type;
-    /// // _must_ use a proxy type. To avoid the need to write these manually, use
+    /// // we _must_ use a proxy type. To avoid the need to write these manually,
-    /// // `local_cache!`, which generates one of the fly.
+    /// // use `local_cache_once!`, which generates one of the fly.
-    /// assert_eq!(local_cache!(request, String::from("hello")), "hello");
+    /// assert_eq!(local_cache_once!(request, String::from("hello")), "hello");
-    /// assert_eq!(local_cache!(request, String::from("goodbye")), "goodbye");
+    /// assert_eq!(local_cache_once!(request, String::from("goodbye")), "goodbye");
+    ///
+    /// // But a macro invocation for the same request always resolves to the
+    /// // first reference as the unique type is generated at compile-time.
+    /// for i in 1..4 {
+    ///     assert_eq!(local_cache_once!(request, i.to_string()), "1");
+    /// }
     /// ```
-    macro_rules! local_cache {
+    macro_rules! local_cache_once {
         ($request:expr, $v:expr $(,)?) => ({
             struct Local<T>(T);
             &$request.local_cache(move || Local($v)).0

core/lib/src/request/request.rs

@@ -703,9 +703,10 @@ impl<'r> Request<'r> {
     /// Different values of the same type _cannot_ be cached without using a
     /// proxy, wrapper type. To avoid the need to write these manually, or for
     /// libraries wishing to store values of public types, use the
-    /// [`local_cache!`](crate::request::local_cache) macro to generate a
+    /// [`local_cache!`](crate::request::local_cache) or
-    /// locally anonymous wrapper type, store, and retrieve the wrapped value
+    /// [`local_cache_once!`](crate::request::local_cache_once) macros to
-    /// from request-local cache.
+    /// generate a locally anonymous wrapper type, store, and retrieve the
+    /// wrapped value from request-local cache.
     ///
     /// # Example
     ///

core/lib/tests/raw-strings-multipart-files-1987.rs

@@ -0,0 +1,48 @@
+#[macro_use] extern crate rocket;
+
+use rocket::form::Form;
+use rocket::http::ContentType;
+use rocket::local::blocking::Client;
+
+#[derive(FromForm)]
+struct Data<'r> {
+    foo: &'r str,
+    bar: &'r str,
+    baz: &'r str,
+}
+
+#[rocket::post("/", data = "<form>")]
+fn form(form: Form<Data<'_>>) -> String {
+    form.foo.to_string() + form.bar + form.baz
+}
+
+#[test]
+fn test_multipart_raw_strings_from_files() {
+    let body = &[
+        "--X-BOUNDARY",
+        r#"Content-Disposition: form-data; name="foo"; filename="foo.txt""#,
+        "Content-Type: text/plain",
+        "",
+        "hi",
+        "--X-BOUNDARY",
+        r#"Content-Disposition: form-data; name="bar"; filename="bar.txt""#,
+        "Content-Type: text/plain",
+        "",
+        "hey",
+        "--X-BOUNDARY",
+        r#"Content-Disposition: form-data; name="baz"; filename="baz.txt""#,
+        "Content-Type: text/plain",
+        "",
+        "bye",
+        "--X-BOUNDARY--",
+        "",
+    ].join("\r\n");
+
+    let client = Client::debug_with(rocket::routes![form]).unwrap();
+    let response = client.post("/")
+        .header("multipart/form-data; boundary=X-BOUNDARY".parse::<ContentType>().unwrap())
+        .body(body)
+        .dispatch();
+
+    assert_eq!(response.into_string().unwrap(), "hiheybye");
+}