Merge pull request awslabs#609 from webern/racey

tough datastore: treat system_time as critical section and document

tough/src/datastore.rs

@@ -2,38 +2,57 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
 use crate::error::{self, Result};
+use chrono::{DateTime, Utc};
 use log::debug;
 use serde::Serialize;
-use snafu::ResultExt;
+use snafu::{ensure, ResultExt};
 use std::fs::{self, File};
 use std::io::{ErrorKind, Read};
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard};
 use tempfile::TempDir;
 
+/// `Datastore` persists TUF metadata files.
 #[derive(Debug, Clone)]
-pub(crate) struct Datastore(Arc<RwLock<DatastorePath>>);
+pub(crate) struct Datastore {
+    /// A lock around retrieving the datastore path.
+    path_lock: Arc<RwLock<DatastorePath>>,
+    /// A lock to treat the system_time function as a critical section.
+    time_lock: Arc<RwLock<()>>,
+}
 
 impl Datastore {
     pub(crate) fn new(path: Option<PathBuf>) -> Result<Self> {
-        // using pattern matching instead of mapping because TempDir::new() can error
-        Ok(Self(Arc::new(RwLock::new(match path {
-            None => DatastorePath::TempDir(TempDir::new().context(error::DatastoreInitSnafu)?),
-            Some(p) => DatastorePath::Path(p),
-        }))))
+        Ok(Self {
+            path_lock: Arc::new(RwLock::new(match path {
+                None => DatastorePath::TempDir(TempDir::new().context(error::DatastoreInitSnafu)?),
+                Some(p) => DatastorePath::Path(p),
+            })),
+            time_lock: Arc::new(RwLock::new(())),
+        })
     }
 
     // Because we are not actually changing the underlying data in the lock, we can ignore when a
     // lock is poisoned.
 
     fn read(&self) -> RwLockReadGuard<'_, DatastorePath> {
-        self.0.read().unwrap_or_else(PoisonError::into_inner)
+        self.path_lock
+            .read()
+            .unwrap_or_else(PoisonError::into_inner)
     }
 
     fn write(&self) -> RwLockWriteGuard<'_, DatastorePath> {
-        self.0.write().unwrap_or_else(PoisonError::into_inner)
+        self.path_lock
+            .write()
+            .unwrap_or_else(PoisonError::into_inner)
     }
 
+    /// Get a reader to a file in the datastore. Caution, this is *not* thread safe. A lock is
+    /// briefly created on the datastore when the read object is created, but it is released at the
+    /// end of this function.
+    ///
+    /// TODO: [provide a thread safe interface](https://github.com/awslabs/tough/issues/602)
+    ///
     pub(crate) fn reader(&self, file: &str) -> Result<Option<impl Read>> {
         let path = self.read().path().join(file);
         match File::open(&path) {
@@ -45,6 +64,7 @@ impl Datastore {
         }
     }
 
+    /// Writes a JSON metadata file in the datastore. This function is thread safe.
     pub(crate) fn create<T: Serialize>(&self, file: &str, value: &T) -> Result<()> {
         let path = self.write().path().join(file);
         serde_json::to_writer_pretty(
@@ -57,6 +77,7 @@ impl Datastore {
         })
     }
 
+    /// Deletes a file from the datastore. This function is thread safe.
     pub(crate) fn remove(&self, file: &str) -> Result<()> {
         let path = self.write().path().join(file);
         debug!("removing '{}'", path.display());
@@ -68,6 +89,46 @@ impl Datastore {
             },
         }
     }
+
+    /// Ensures that system time has not stepped backward since it was last sampled. This function
+    /// is protected by a lock guard to ensure thread safety.
+    pub(crate) fn system_time(&self) -> Result<DateTime<Utc>> {
+        // Treat this function as a critical section. This lock is not used for anything else.
+        let lock = self.time_lock.write().map_err(|e| {
+            // Painful error type that has a reference and lifetime. Convert it to a message string.
+            error::DatastoreTimeLockSnafu {
+                message: e.to_string(),
+            }
+            .build()
+        })?;
+
+        let file = "latest_known_time.json";
+        // Load the latest known system time, if it exists
+        let poss_latest_known_time = self
+            .reader(file)?
+            .map(serde_json::from_reader::<_, DateTime<Utc>>);
+
+        // Get 'current' system time
+        let sys_time = Utc::now();
+
+        if let Some(Ok(latest_known_time)) = poss_latest_known_time {
+            // Make sure the sampled system time did not go back in time
+            ensure!(
+                sys_time >= latest_known_time,
+                error::SystemTimeSteppedBackwardSnafu {
+                    sys_time,
+                    latest_known_time
+                }
+            );
+        }
+        // Store the latest known time
+        // Serializes RFC3339 time string and store to datastore
+        self.create(file, &sys_time)?;
+
+        // Explicitly drop the lock to avoid any compiler optimization.
+        drop(lock);
+        Ok(sys_time)
+    }
 }
 
 /// Because `TempDir` is an RAII object, we need to hold on to it. This private enum allows us to

tough/src/error.rs

@@ -71,6 +71,9 @@ pub enum Error {
         backtrace: Backtrace,
     },
 
+    #[snafu(display("Failure to obtain a lock in the system_time function: {}", message))]
+    DatastoreTimeLock { message: String },
+
     #[snafu(display("Failed to create directory '{}': {}", path.display(), source))]
     DirCreate {
         path: PathBuf,

tough/src/lib.rs

@@ -430,7 +430,7 @@ impl Repository {
         // Check for repository metadata expiration.
         if self.expiration_enforcement == ExpirationEnforcement::Safe {
             ensure!(
-                system_time(&self.datastore)? < self.earliest_expiration,
+                self.datastore.system_time()? < self.earliest_expiration,
                 error::ExpiredMetadataSnafu {
                     role: self.earliest_expiration_role
                 }
@@ -587,38 +587,11 @@ pub(crate) fn encode_filename<S: AsRef<str>>(name: S) -> String {
     utf8_percent_encode(name.as_ref(), &CHARACTERS_TO_ESCAPE).to_string()
 }
 
-/// Ensures that system time has not stepped backward since it was last sampled
-fn system_time(datastore: &Datastore) -> Result<DateTime<Utc>> {
-    let file = "latest_known_time.json";
-    // Load the latest known system time, if it exists
-    let poss_latest_known_time = datastore
-        .reader(file)?
-        .map(serde_json::from_reader::<_, DateTime<Utc>>);
-
-    // Get 'current' system time
-    let sys_time = Utc::now();
-
-    if let Some(Ok(latest_known_time)) = poss_latest_known_time {
-        // Make sure the sampled system time did not go back in time
-        ensure!(
-            sys_time >= latest_known_time,
-            error::SystemTimeSteppedBackwardSnafu {
-                sys_time,
-                latest_known_time
-            }
-        );
-    }
-    // Store the latest known time
-    // Serializes RFC3339 time string and store to datastore
-    datastore.create(file, &sys_time)?;
-    Ok(sys_time)
-}
-
 /// TUF v1.0.16, 5.2.9, 5.3.3, 5.4.5, 5.5.4, The expiration timestamp in the `[metadata]` file MUST
 /// be higher than the fixed update start time.
 fn check_expired<T: Role>(datastore: &Datastore, role: &T) -> Result<()> {
     ensure!(
-        system_time(datastore)? <= role.expires(),
+        datastore.system_time()? <= role.expires(),
         error::ExpiredMetadataSnafu { role: T::TYPE }
     );
     Ok(())