Fix safety issues & comments

- The previous `on_request` was unsafe, now uses a separate
  `filter_request` method to ensure `error` only contains
  immutable borrows of `Request`.
- Added full safety comments and arguements to `erased`.
- Added extra logic to upgrade, to prevent upgrading if an
  error has been thrown.

Author: the10thWiz

core/lib/src/erased.rs

@@ -35,12 +35,40 @@ impl Drop for ErasedRequest {
     fn drop(&mut self) { }
 }
 
+pub struct ErasedError<'r> {
+    error: Option<Pin<Box<dyn TypedError<'r> + 'r>>>,
+}
+
+impl<'r> ErasedError<'r> {
+    pub fn new() -> Self {
+        Self { error: None }        
+    }
+
+    pub fn write(&mut self, error: Option<Box<dyn TypedError<'r> + 'r>>) {
+        // SAFETY: To meet the requirements of `Pin`, we never drop
+        // the inner Box. This is enforced by only allowing writing
+        // to the Option when it is None.
+        assert!(self.error.is_none());
+        if let Some(error) = error {
+            self.error = Some(unsafe { Pin::new_unchecked(error) });
+        }
+    }
+
+    pub fn is_some(&self) -> bool {
+        self.error.is_some()
+    }
+
+    pub fn get(&'r self) -> Option<&'r dyn TypedError<'r>> {
+        self.error.as_ref().map(|e| &**e)
+    }
+}
+
 // TODO: #[derive(Debug)]
 pub struct ErasedResponse {
     // XXX: SAFETY: This (dependent) field must come first due to drop order!
     response: Response<'static>,
-    // XXX: SAFETY: This (dependent) field must come first due to drop order!
-    error: Option<Box<dyn TypedError<'static> + 'static>>,
+    // XXX: SAFETY: This (dependent) field must come second due to drop order!
+    error: ErasedError<'static>,
     _request: Arc<ErasedRequest>,
 }
 
@@ -71,8 +99,13 @@ impl ErasedRequest {
         let parts: Box<Parts> = Box::new(parts);
         let request: Request<'_> = {
             let rocket: &Rocket<Orbit> = &rocket;
+            // SAFETY: The `Request` can borrow from `Rocket` because it has a stable
+            // address (due to `Arc`)  and it is kept alive by the containing
+            // `ErasedRequest`. The `Request` is always dropped before the
+            // `Arc<Rocket>` due to drop order.
             let rocket: &'static Rocket<Orbit> = unsafe { transmute(rocket) };
             let parts: &Parts = &parts;
+            // SAFETY: Same as above, but for `Box<Parts>`.
             let parts: &'static Parts = unsafe { transmute(parts) };
             constructor(rocket, parts)
         };
@@ -92,46 +125,54 @@ impl ErasedRequest {
             &'r Rocket<Orbit>,
             &'r mut Request<'x>,
             &'r mut Data<'x>,
-            &'r mut Option<Box<dyn TypedError<'r> + 'r>>,
+            &'r mut ErasedError<'r>,
         ) -> BoxFuture<'r, T>,
         dispatch: impl for<'r> FnOnce(
             T,
             &'r Rocket<Orbit>,
             &'r Request<'r>,
             Data<'r>,
-            &'r mut Option<Box<dyn TypedError<'r> + 'r>>,
+            &'r mut ErasedError<'r>,
         ) -> BoxFuture<'r, Response<'r>>,
     ) -> ErasedResponse
         where T: Send + Sync + 'static,
               D: for<'r> Into<RawStream<'r>>
     {
-        let mut error_ptr: Option<Box<dyn TypedError<'static> + 'static>> = None;
         let mut data: Data<'_> = Data::from(raw_stream);
+        // SAFETY: At this point, ErasedRequest contains a request, which is permitted
+        // to borrow from `Rocket` and `Parts`. They both have stable addresses (due to
+        // `Arc` and `Box`), and the Request will be dropped first (due to drop order).
+        // SAFETY: Here, we place the `ErasedRequest` (i.e. the `Request`) behind an `Arc` (TODO: Why not Box?)
+        // to ensure it has a stable address, and we again use drop order to ensure the `Request`
+        // is dropped before the values that can borrow from it.
         let mut parent = Arc::new(self);
+        // SAFETY: This error is permitted to borrow from the `Request` (as well as `Rocket` and `Parts`).
+        let mut error = ErasedError { error: None };
         let token: T = {
             let parent: &mut ErasedRequest = Arc::get_mut(&mut parent).unwrap();
             let rocket: &Rocket<Orbit> = &parent._rocket;
             let request: &mut Request<'_> = &mut parent.request;
             let data: &mut Data<'_> = &mut data;
-            // SAFETY: TODO: Same as below
-            preprocess(rocket, request, data, unsafe { transmute(&mut error_ptr) }).await
+            // SAFETY: As below, `error` must be reborrowed with the correct lifetimes.
+            preprocess(rocket, request, data, unsafe { transmute(&mut error) }).await
         };
 
         let parent = parent;
         let response: Response<'_> = {
             let parent: &ErasedRequest = &parent;
+            // SAFETY: This static reference is immediatly reborrowed for the correct lifetime.
+            // The Response type is permitted to borrow from the `Request`, `Rocket`, `Parts`, and
+            // `error`. All of these types have stable addresses, and will not be dropped until
+            // after Response, due to drop order.
             let parent: &'static ErasedRequest = unsafe { transmute(parent) };
             let rocket: &Rocket<Orbit> = &parent._rocket;
             let request: &Request<'_> = &parent.request;
-            // SAFETY: TODO: error_ptr is transmuted into the same type, with the
-            // same lifetime as the request.
-            // It is kept alive by the erased response, so that the response
-            // type can borrow from it
-            dispatch(token, rocket, request, data, unsafe { transmute(&mut error_ptr)}).await
+            // SAFETY: As above, `error` must be reborrowed with the correct lifetimes.
+            dispatch(token, rocket, request, data, unsafe { transmute(&mut error) }).await
         };
 
         ErasedResponse {
-            error: error_ptr,
+            error,
             _request: parent,
             response,
         }
@@ -159,9 +200,21 @@ impl ErasedResponse {
             &'a mut Response<'r>,
         ) -> Option<(T, Box<dyn IoHandler + 'r>)>
     ) -> Option<(T, ErasedIoHandler)> {
+        // SAFETY: If an error has been thrown, the `IoHandler` could
+        // technically borrow from it, so we must ensure that this is
+        // not the case. This could be handled safely by changing `error`
+        // to be an `Arc` internally, and cloning the Arc to get a copy
+        // (like `ErasedRequest`), however it's unclear this is actually
+        // useful, and we can avoid paying the cost of an `Arc`
+        if self.error.is_some() {
+            warn!("Attempting to upgrade after throwing a typed error is not supported");
+            return None;
+        }
         let parent: Arc<ErasedRequest> = self._request.clone();
         let io: Option<(T, Box<dyn IoHandler + '_>)> = {
             let parent: &ErasedRequest = &parent;
+            // SAFETY: As in other cases, the request is kept alive by the `Erased...`
+            // type.
             let parent: &'static ErasedRequest = unsafe { transmute(parent) };
             let request: &Request<'_> = &parent.request;
             constructor(request, &mut self.response)

core/lib/src/fairing/ad_hoc.rs

@@ -62,6 +62,10 @@ enum AdHocKind {
 
     /// An ad-hoc **request** fairing. Called when a request is received.
     Request(Box<dyn for<'a, 'b> Fn(&'a mut Request<'_>, &'b mut Data<'_>)
+        -> BoxFuture<'a, ()> + Send + Sync + 'static>),
+
+    /// An ad-hoc **request_filter** fairing. Called when a request is received.
+    RequestFilter(Box<dyn for<'a, 'b> Fn(&'a Request<'_>, &'b Data<'_>)
         -> BoxFuture<'a, Result<(), Box<dyn TypedError<'a> + 'a>>> + Send + Sync + 'static>),
 
     /// An ad-hoc **response** fairing. Called when a response is ready to be
@@ -156,11 +160,35 @@ impl AdHoc {
     /// ```
     pub fn on_request<F: Send + Sync + 'static>(name: &'static str, f: F) -> AdHoc
         where F: for<'a, 'b> Fn(&'a mut Request<'_>, &'b mut Data<'_>)
-                        -> BoxFuture<'a, Result<(), Box<dyn TypedError<'a> + 'a>>>
+                        -> BoxFuture<'a, ()>
     {
         AdHoc { name, kind: AdHocKind::Request(Box::new(f)) }
     }
 
+    /// Constructs an `AdHoc` request fairing named `name`. The function `f`
+    /// will be called and the returned `Future` will be `await`ed by Rocket
+    /// when a new request is received.
+    ///
+    /// # Example
+    ///
+    /// ```rust
+    /// use rocket::fairing::AdHoc;
+    ///
+    /// // The no-op request fairing.
+    /// let fairing = AdHoc::on_request("Dummy", |req, data| {
+    ///     Box::pin(async move {
+    ///         // do something with the request and data...
+    /// #       let (_, _) = (req, data);
+    ///     })
+    /// });
+    /// ```
+    pub fn filter_request<F: Send + Sync + 'static>(name: &'static str, f: F) -> AdHoc
+        where F: for<'a, 'b> Fn(&'a Request<'_>, &'b Data<'_>)
+                        -> BoxFuture<'a, Result<(), Box<dyn TypedError<'a> + 'a>>>
+    {
+        AdHoc { name, kind: AdHocKind::RequestFilter(Box::new(f)) }
+    }
+
     // FIXME(rustc): We'd like to allow passing `async fn` to these methods...
     // https://github.com/rust-lang/rust/issues/64552#issuecomment-666084589
 
@@ -379,12 +407,10 @@ impl AdHoc {
                 let _ = self.routes(rocket);
             }
 
-            async fn on_request<'r>(&self, req: &'r mut Request<'_>, _: &mut Data<'_>)
-                -> Result<(), Box<dyn TypedError<'r> + 'r>>
-            {
+            async fn on_request<'r>(&self, req: &'r mut Request<'_>, _: &mut Data<'_>) {
                 // If the URI has no trailing slash, it routes as before.
                 if req.uri().is_normalized_nontrailing() {
-                    return Ok(());
+                    return;
                 }
 
                 // Otherwise, check if there's a route that matches the request
@@ -397,7 +423,6 @@ impl AdHoc {
                         "incoming request URI normalized for compatibility");
                     req.set_uri(normalized);
                 }
-                Ok(())
             }
         }
 
@@ -412,6 +437,7 @@ impl Fairing for AdHoc {
             AdHocKind::Ignite(_) => Kind::Ignite,
             AdHocKind::Liftoff(_) => Kind::Liftoff,
             AdHocKind::Request(_) => Kind::Request,
+            AdHocKind::RequestFilter(_) => Kind::RequestFilter,
             AdHocKind::Response(_) => Kind::Response,
             AdHocKind::Shutdown(_) => Kind::Shutdown,
         };
@@ -432,10 +458,16 @@ impl Fairing for AdHoc {
         }
     }
 
-    async fn on_request<'r>(&self, req: &'r mut Request<'_>, data: &mut Data<'_>)
+    async fn on_request<'r>(&self, req: &'r mut Request<'_>, data: &mut Data<'_>) {
+        if let AdHocKind::Request(ref f) = self.kind {
+            f(req, data).await
+        }
+    }
+
+    async fn filter_request<'r>(&self, req: &'r Request<'_>, data: &Data<'_>)
         -> Result<(), Box<dyn TypedError<'r> + 'r>>
     {
-        if let AdHocKind::Request(ref f) = self.kind {
+        if let AdHocKind::RequestFilter(ref f) = self.kind {
             f(req, data).await
         } else {
             Ok(())

core/lib/src/fairing/fairings.rs

@@ -1,7 +1,6 @@
 use std::collections::HashSet;
-use std::mem::transmute;
 
-use crate::catcher::TypedError;
+use crate::erased::ErasedError;
 use crate::{Rocket, Request, Response, Data, Build, Orbit};
 use crate::fairing::{Fairing, Info, Kind};
 
@@ -17,6 +16,7 @@ pub struct Fairings {
     ignite: Vec<usize>,
     liftoff: Vec<usize>,
     request: Vec<usize>,
+    filter_request: Vec<usize>,
     response: Vec<usize>,
     shutdown: Vec<usize>,
 }
@@ -45,6 +45,7 @@ impl Fairings {
         self.ignite.iter()
             .chain(self.liftoff.iter())
             .chain(self.request.iter())
+            .chain(self.filter_request.iter())
             .chain(self.response.iter())
             .chain(self.shutdown.iter())
     }
@@ -106,6 +107,7 @@ impl Fairings {
         if this_info.kind.is(Kind::Ignite) { self.ignite.push(index); }
         if this_info.kind.is(Kind::Liftoff) { self.liftoff.push(index); }
         if this_info.kind.is(Kind::Request) { self.request.push(index); }
+        if this_info.kind.is(Kind::RequestFilter) { self.filter_request.push(index); }
         if this_info.kind.is(Kind::Response) { self.response.push(index); }
         if this_info.kind.is(Kind::Shutdown) { self.shutdown.push(index); }
     }
@@ -153,17 +155,26 @@ impl Fairings {
         &self,
         req: &'r mut Request<'_>,
         data: &mut Data<'_>,
-        error: &mut Option<Box<dyn TypedError<'_> + '_>>,
     ) {
         for fairing in iter!(self.request) {
-            // invoke_fairing(fairing, req, data, error)?;
-            match fairing.on_request(req, data).await {
+            fairing.on_request(req, data).await;
+        }
+    }
+
+    #[inline(always)]
+    pub async fn handle_filter<'r>(
+        &self,
+        req: &'r Request<'_>,
+        data: &Data<'_>,
+        error: &mut ErasedError<'r>,
+    ) {
+        for fairing in iter!(self.filter_request) {
+            match fairing.filter_request(req, data).await {
                 Ok(()) => (),
                 Err(e) => {
-                    // TODO: Safety arguement
-                    // Generally, error is None at the start (hence no borrows),
-                    // and we always return immediatly with this value.
-                    *error = Some(unsafe { transmute(e) });
+                    // SAFETY: `e` can only contain *immutable* borrows of
+                    // `req`.
+                    error.write(Some(e));
                     return;
                 },
             }

core/lib/src/fairing/info_kind.rs

@@ -64,15 +64,18 @@ impl Kind {
     /// `Kind` flag representing a request for a 'request' callback.
     pub const Request: Kind = Kind(1 << 2);
 
+    /// `Kind` flag representing a request for a 'filter_request' callback.
+    pub const RequestFilter: Kind = Kind(1 << 3);
+
     /// `Kind` flag representing a request for a 'response' callback.
-    pub const Response: Kind = Kind(1 << 3);
+    pub const Response: Kind = Kind(1 << 4);
 
     /// `Kind` flag representing a request for a 'shutdown' callback.
-    pub const Shutdown: Kind = Kind(1 << 4);
+    pub const Shutdown: Kind = Kind(1 << 5);
 
     /// `Kind` flag representing a
     /// [singleton](crate::fairing::Fairing#singletons) fairing.
-    pub const Singleton: Kind = Kind(1 << 5);
+    pub const Singleton: Kind = Kind(1 << 6);
 
     /// Returns `true` if `self` is a superset of `other`. In other words,
     /// returns `true` if all of the kinds in `other` are also in `self`.

core/lib/src/fairing/mod.rs

@@ -150,9 +150,18 @@ pub type Result<T = Rocket<Build>, E = Rocket<Build>> = std::result::Result<T, E
 ///     [`Request`] and [`Data`] structures but has not routed the request. A
 ///     request callback can modify the request at will and [`Data::peek()`]
 ///     into the incoming data. It may not, however, abort or respond directly
-///     to the request; these issues are better handled via [request guards] or
-///     via response callbacks. Any modifications to a request are persisted and
-///     can potentially alter how a request is routed.
+///     to the request; these issues are better handled via [request guards],
+///     Request filters, or via response callbacks. Any modifications to a
+///     request are persisted and can potentially alter how a request is routed.
+///
+///   * **<a name="filter_request">Request filter</a> (`filter_request`)**
+///
+///     A request callback, represented by the [`Fairing::filter_request()`] method,
+///     called after `on_request` callbacks have run, but before any handlers have
+///     been attempted. This type of fairing can choose to prematurly reject requests,
+///     skipping handlers all together, and moving it straight to error handling. This
+///     should generally only be used to apply filter that apply to the entire server,
+///     e.g. CORS processing.
 ///
 ///   * **<a name="response">Response</a> (`on_response`)**
 ///
@@ -502,7 +511,26 @@ pub trait Fairing: Send + Sync + Any + 'static {
     /// ## Default Implementation
     ///
     /// The default implementation of this method does nothing.
-    async fn on_request<'r>(&self, _req: &'r mut Request<'_>, _data: &mut Data<'_>)
+    async fn on_request<'r>(&self, _req: &'r mut Request<'_>, _data: &mut Data<'_>) { }
+
+    /// The request filter callback.
+    ///
+    /// See [Fairing Callbacks](#filter_request) for complete semantics.
+    ///
+    /// This method is called when a new request is received if `Kind::RequestFilter`
+    /// is in the `kind` field of the `Info` structure for this fairing. The
+    /// `&Request` parameter is the incoming request, and the `&Data`
+    /// parameter is the incoming data in the request.
+    ///
+    /// If this method returns `Ok`, the request routed as normal (assuming no other
+    /// fairing filters it). Otherwise, the request is routed to an error handler
+    /// based on the error type returned.
+    ///
+    /// ## Default Implementation
+    ///
+    /// The default implementation of this method does not filter any request,
+    /// by always returning `Ok(())`
+    async fn filter_request<'r>(&self, _req: &'r Request<'_>, _data: &Data<'_>)
         -> Result<(), Box<dyn TypedError<'r> + 'r>>
     { Ok(()) }
 
@@ -554,10 +582,15 @@ impl<T: Fairing + ?Sized> Fairing for std::sync::Arc<T> {
     }
 
     #[inline]
-    async fn on_request<'r>(&self, req: &'r mut Request<'_>, data: &mut Data<'_>)
+    async fn on_request<'r>(&self, req: &'r mut Request<'_>, data: &mut Data<'_>) {
+        (self as &T).on_request(req, data).await
+    }
+
+    #[inline]
+    async fn filter_request<'r>(&self, req: &'r Request<'_>, data: &Data<'_>)
         -> Result<(), Box<dyn TypedError<'r> + 'r>>
     {
-        (self as &T).on_request(req, data).await
+        (self as &T).filter_request(req, data).await
     }
 
     #[inline]