From 276a5a3ee444bcfb57ac5b8927129c000b5a91fb Mon Sep 17 00:00:00 2001
From: Aaron Hill <aa1ronham@gmail.com>
Date: Wed, 29 Jan 2020 07:05:08 -0500
Subject: [PATCH] Replace `UnsafeCell` with `Cell` in `DateServiceInner`
 (#1325)

* Replace `UnsafeCell` with `Cell` in `DateServiceInner`

This ensures that it's impossible to cause undefined behavior by
accidentally violating Rust's aliasing rules (e.g. passing a closure to
`set_date` which ends up invoking `reset` or `update` on the inner
`DateServiceInner`).

There might be a tiny amount of overhead from copying the `Option<(Date,
Instant)>` rather than taking a reference, but it shouldn't be
measurable.

Since the wrapped type is `Copy`, a `Cell` can be used, avoiding the
runtime overhead of a `RefCell`.

Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>
---
 actix-http/src/config.rs | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/actix-http/src/config.rs b/actix-http/src/config.rs
index 50322bf2..a38a80e7 100644
--- a/actix-http/src/config.rs
+++ b/actix-http/src/config.rs
@@ -1,4 +1,4 @@
-use std::cell::UnsafeCell;
+use std::cell::Cell;
 use std::fmt::Write;
 use std::rc::Rc;
 use std::time::Duration;
@@ -228,24 +228,24 @@ impl fmt::Write for Date {
 struct DateService(Rc<DateServiceInner>);
 
 struct DateServiceInner {
-    current: UnsafeCell<Option<(Date, Instant)>>,
+    current: Cell<Option<(Date, Instant)>>,
 }
 
 impl DateServiceInner {
     fn new() -> Self {
         DateServiceInner {
-            current: UnsafeCell::new(None),
+            current: Cell::new(None),
         }
     }
 
     fn reset(&self) {
-        unsafe { (&mut *self.current.get()).take() };
+        self.current.take();
     }
 
     fn update(&self) {
         let now = Instant::now();
         let date = Date::new();
-        *(unsafe { &mut *self.current.get() }) = Some((date, now));
+        self.current.set(Some((date, now)));
     }
 }
 
@@ -255,7 +255,7 @@ impl DateService {
     }
 
     fn check_date(&self) {
-        if unsafe { (&*self.0.current.get()).is_none() } {
+        if self.0.current.get().is_none() {
             self.0.update();
 
             // periodic date update
@@ -269,12 +269,12 @@ impl DateService {
 
     fn now(&self) -> Instant {
         self.check_date();
-        unsafe { (&*self.0.current.get()).as_ref().unwrap().1 }
+        self.0.current.get().unwrap().1
     }
 
     fn set_date<F: FnMut(&Date)>(&self, mut f: F) {
         self.check_date();
-        f(&unsafe { (&*self.0.current.get()).as_ref().unwrap().0 })
+        f(&self.0.current.get().unwrap().0);
     }
 }
 
@@ -282,6 +282,19 @@ impl DateService {
 mod tests {
     use super::*;
 
+
+    // Test modifying the date from within the closure
+    // passed to `set_date`
+    #[test]
+    fn test_evil_date() {
+        let service = DateService::new();
+        // Make sure that `check_date` doesn't try to spawn a task
+        service.0.update();
+        service.set_date(|_| {
+            service.0.reset()
+        });
+    }
+
     #[test]
     fn test_date_len() {
         assert_eq!(DATE_VALUE_LENGTH, "Sun, 06 Nov 1994 08:49:37 GMT".len());