Bug 1429796 - cert_storage: create rkv environment and store only once to avoid races r=mgoodwin,jcj
☠☠ backed out by 91403c24fee3 ☠ ☠
authorDana Keeler <dkeeler@mozilla.com>
Mon, 18 Mar 2019 20:08:30 +0000
changeset 465074 b0d08863f7a5fc08a3c0709b5e7151d80ae18261
parent 465073 1bd54f8dfd9e877154a8a22992008ef74910fe37
child 465075 339f5b718539f0226cb1c9ac638b358d4fe8113a
push id35732
push useropoprus@mozilla.com
push dateWed, 20 Mar 2019 10:52:37 +0000
treeherdermozilla-central@708979f9c3f3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgoodwin, jcj
bugs1429796, 1535752
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1429796 - cert_storage: create rkv environment and store only once to avoid races r=mgoodwin,jcj This patch also base64-decodes the API inputs before storing in the DB in anticipation of being able to pass binary data directly (bug 1535752). Differential Revision: https://phabricator.services.mozilla.com/D23430
security/manager/ssl/cert_storage/src/lib.rs
--- a/security/manager/ssl/cert_storage/src/lib.rs
+++ b/security/manager/ssl/cert_storage/src/lib.rs
@@ -24,23 +24,30 @@ use std::path::PathBuf;
 use std::slice;
 use std::str;
 use std::sync::RwLock;
 use std::time::{Duration, SystemTime};
 use style::gecko_bindings::structs::nsresult;
 use xpcom::interfaces::{nsICertStorage, nsIFile, nsIObserver, nsIPrefBranch, nsISupports};
 use xpcom::{nsIID, GetterAddrefs, RefPtr, XpCom};
 
-use rkv::{Rkv, StoreOptions, Value};
+use rkv::{Rkv, SingleStore, StoreOptions, Value};
 
 const PREFIX_REV_IS: &str = "is";
 const PREFIX_REV_SPK: &str = "spk";
 const PREFIX_CRLITE: &str = "crlite";
 const PREFIX_WL: &str = "wl";
 
+fn make_key(prefix: &str, part_a: &[u8], part_b: &[u8]) -> Vec<u8> {
+    let mut key = prefix.as_bytes().to_owned();
+    key.extend_from_slice(part_a);
+    key.extend_from_slice(part_b);
+    key
+}
+
 #[allow(non_camel_case_types, non_snake_case)]
 
 /// `SecurityStateError` is a type to represent errors in accessing or
 /// modifying security state.
 #[derive(Debug)]
 pub struct SecurityStateError {
     message: String,
 }
@@ -52,28 +59,34 @@ impl<T: Display> From<T> for SecuritySta
         SecurityStateError {
             message: format!("{}", err),
         }
     }
 }
 
 /// `SecurityState`
 pub struct SecurityState {
-    path: PathBuf,
+    env: Rkv,
+    store: SingleStore,
     int_prefs: HashMap<String, i32>,
 }
 
 impl SecurityState {
     pub fn new(profile_path: PathBuf) -> Result<SecurityState, SecurityStateError> {
         let mut store_path = profile_path.clone();
         store_path.push("security_state");
 
         create_dir_all(store_path.as_path())?;
+        let env = Rkv::new(store_path.as_path())?;
+        let mut options = StoreOptions::create();
+        options.create = true;
+        let store = env.open_single("cert_storage", options)?;
         let mut ss = SecurityState {
-            path: store_path,
+            env: env,
+            store: store,
             int_prefs: HashMap::new(),
         };
 
         let mut revocations_path = profile_path;
         revocations_path.push("revocations.txt");
 
         // if the profile has a revocations.txt, migrate it and remove the file
         if revocations_path.exists() {
@@ -83,199 +96,183 @@ impl SecurityState {
         Ok(ss)
     }
 
     fn migrate(&mut self, revocations_path: &PathBuf) -> Result<(), SecurityStateError> {
         let f = File::open(revocations_path)?;
         let file = BufReader::new(f);
 
         // Add the data from revocations.txt
-        let mut dn: Option<String> = None;
-        let mut l;
+        let mut dn: Option<Vec<u8>> = None;
         for line in file.lines() {
-            l = match line.map_err(|_| SecurityStateError::from("io error reading line data")) {
-                Ok(data) => data.to_owned(),
+            let l = match line.map_err(|_| SecurityStateError::from("io error reading line data")) {
+                Ok(data) => data,
                 Err(e) => return Err(e),
             };
-            let l_sans_prefix = match l.len() {
-                0 => "".to_owned(),
-                _ => l[1..].to_owned(),
+            if l.len() == 0 || l.starts_with("#") {
+                continue;
+            }
+            let leading_char = match l.chars().next() {
+                Some(c) => c,
+                None => {
+                    return Err(SecurityStateError::from(
+                        "couldn't get char from non-empty str?",
+                    ));
+                }
             };
-            // In future, we can maybe log migration failures. For now, ignore the error
-            // and attempt to continue.
-            let _ = match l.chars().next() {
-                Some('#') => Ok(()),
-                Some('\t') => match &dn {
-                    Some(name) => self.set_revocation_by_subject_and_pub_key(
-                        &name,
+            // In future, we can maybe log migration failures. For now, ignore decoding and storage
+            // errors and attempt to continue.
+            // Check if we have a new DN
+            if leading_char != '\t' && leading_char != ' ' {
+                if let Ok(decoded_dn) = base64::decode(&l) {
+                    dn = Some(decoded_dn);
+                }
+                continue;
+            }
+            let l_sans_prefix = match base64::decode(&l[1..]) {
+                Ok(decoded) => decoded,
+                Err(_) => continue,
+            };
+            if let Some(name) = &dn {
+                if leading_char == '\t' {
+                    let _ = self.set_revocation_by_subject_and_pub_key(
+                        name,
                         &l_sans_prefix,
                         nsICertStorage::STATE_ENFORCE as i16,
-                    ),
-                    None => Err(SecurityStateError::from("pubkey with no subject")),
-                },
-                Some(' ') => match &dn {
-                    Some(name) => self.set_revocation_by_issuer_and_serial(
-                        &name,
+                    );
+                } else {
+                    let _ = self.set_revocation_by_issuer_and_serial(
+                        name,
                         &l_sans_prefix,
                         nsICertStorage::STATE_ENFORCE as i16,
-                    ),
-                    None => Err(SecurityStateError::from("serial with no issuer")),
-                },
-                Some(_) => {
-                    dn = Some(l);
-                    Ok(())
+                    );
                 }
-                None => Ok(()),
-            };
+            }
         }
 
         Ok(())
     }
 
-    fn write_entry(&mut self, key: &str, value: i16) -> Result<(), SecurityStateError> {
-        let p = self.path.as_path();
-        let env = Rkv::new(p)?;
-        let store = env.open_single("cert_storage", StoreOptions::create())?;
-
-        let mut writer = env.write()?;
-        store.put(&mut writer, key, &Value::I64(value as i64))?;
+    fn write_entry(&mut self, key: &[u8], value: i16) -> Result<(), SecurityStateError> {
+        let mut writer = self.env.write()?;
+        self.store
+            .put(&mut writer, key, &Value::I64(value as i64))?;
         writer.commit()?;
         Ok(())
     }
 
-    fn read_entry(&self, key: &str) -> Result<Option<i16>, SecurityStateError> {
-        // TODO: figure out a way to de-dupe getting the env / store
-        let p = self.path.as_path();
-        let env = Rkv::new(p)?;
-        let store = env.open_single("cert_storage", StoreOptions::create())?;
-
-        let reader = env.read()?;
-        match store.get(&reader, key) {
-            // There's no tidy way in rkv::Value to get an owned value. The
-            // only way I can see to get such functionality is to go via
-            // primitives.
+    fn read_entry(&self, key: &[u8]) -> Result<Option<i16>, SecurityStateError> {
+        let reader = self.env.read()?;
+        match self.store.get(&reader, key) {
             Ok(Some(Value::I64(i)))
                 if i <= (std::i16::MAX as i64) && i >= (std::i16::MIN as i64) =>
             {
                 Ok(Some(i as i16))
             }
             Ok(None) => Ok(None),
             _ => Err(SecurityStateError::from(
                 "There was a problem getting the value",
             )),
         }
     }
 
     pub fn set_revocation_by_issuer_and_serial(
         &mut self,
-        issuer: &str,
-        serial: &str,
+        issuer: &[u8],
+        serial: &[u8],
         state: i16,
     ) -> Result<(), SecurityStateError> {
-        self.write_entry(&format!("{}:{}:{}", PREFIX_REV_IS, issuer, serial), state)
+        self.write_entry(&make_key(PREFIX_REV_IS, issuer, serial), state)
     }
 
     pub fn set_revocation_by_subject_and_pub_key(
         &mut self,
-        subject: &str,
-        pub_key_hash: &str,
+        subject: &[u8],
+        pub_key_hash: &[u8],
         state: i16,
     ) -> Result<(), SecurityStateError> {
-        self.write_entry(
-            &format!("{}:{}:{}", PREFIX_REV_SPK, subject, pub_key_hash),
-            state,
-        )
+        self.write_entry(&make_key(PREFIX_REV_SPK, subject, pub_key_hash), state)
     }
 
     pub fn set_enrollment(
         &mut self,
-        issuer: &str,
-        serial: &str,
+        issuer: &[u8],
+        serial: &[u8],
         state: i16,
     ) -> Result<(), SecurityStateError> {
-        self.write_entry(&format!("{}:{}:{}", PREFIX_CRLITE, issuer, serial), state)
+        self.write_entry(&make_key(PREFIX_CRLITE, issuer, serial), state)
     }
 
     pub fn set_whitelist(
         &mut self,
-        issuer: &str,
-        serial: &str,
+        issuer: &[u8],
+        serial: &[u8],
         state: i16,
     ) -> Result<(), SecurityStateError> {
-        self.write_entry(&format!("{}:{}:{}", PREFIX_WL, issuer, serial), state)
+        self.write_entry(&make_key(PREFIX_WL, issuer, serial), state)
     }
 
     pub fn get_revocation_state(
         &self,
-        issuer: &str,
-        serial: &str,
-        subject: &str,
-        pub_key: &str,
+        issuer: &[u8],
+        serial: &[u8],
+        subject: &[u8],
+        pub_key: &[u8],
     ) -> Result<i16, SecurityStateError> {
-        let pub_key_hash = match base64::decode(pub_key) {
-            Ok(pk) => {
-                let mut digest = Sha256::default();
-                digest.input(&pk);
-                let digest_result = digest.result();
-                base64::encode(&digest_result)
-            }
-            Err(_) => {
-                return Err(SecurityStateError::from(
-                    "problem base64 decoding public key",
-                ));
-            }
-        };
+        let mut digest = Sha256::default();
+        digest.input(pub_key);
+        let pub_key_hash = digest.result();
 
-        let subject_pubkey = format!("{}:{}:{}", PREFIX_REV_SPK, subject, pub_key_hash);
-        let issuer_serial = format!("{}:{}:{}", PREFIX_REV_IS, issuer, serial);
+        let subject_pubkey = make_key(PREFIX_REV_SPK, subject, &pub_key_hash);
+        let issuer_serial = make_key(PREFIX_REV_IS, issuer, serial);
 
         let st: i16 = match self.read_entry(&issuer_serial) {
             Ok(Some(value)) => value,
             Ok(None) => nsICertStorage::STATE_UNSET as i16,
             Err(_) => {
                 return Err(SecurityStateError::from(
                     "problem reading revocation state (from issuer / serial)",
-                ))
+                ));
             }
         };
 
         if st != nsICertStorage::STATE_UNSET as i16 {
             return Ok(st);
         }
 
         match self.read_entry(&subject_pubkey) {
             Ok(Some(value)) => Ok(value),
             Ok(None) => Ok(nsICertStorage::STATE_UNSET as i16),
             Err(_) => {
                 return Err(SecurityStateError::from(
                     "problem reading revocation state (from subject / pubkey)",
-                ))
+                ));
             }
         }
     }
 
     pub fn get_enrollment_state(
         &self,
-        issuer: &str,
-        serial: &str,
+        issuer: &[u8],
+        serial: &[u8],
     ) -> Result<i16, SecurityStateError> {
-        let issuer_serial = format!("{}:{}:{}", PREFIX_CRLITE, issuer, serial);
+        let issuer_serial = make_key(PREFIX_CRLITE, issuer, serial);
         match self.read_entry(&issuer_serial) {
             Ok(Some(value)) => Ok(value),
             Ok(None) => Ok(nsICertStorage::STATE_UNSET as i16),
             Err(_) => return Err(SecurityStateError::from("problem reading enrollment state")),
         }
     }
 
     pub fn get_whitelist_state(
         &self,
-        issuer: &str,
-        serial: &str,
+        issuer: &[u8],
+        serial: &[u8],
     ) -> Result<i16, SecurityStateError> {
-        let issuer_serial = format!("{}:{}:{}", PREFIX_WL, issuer, serial);
+        let issuer_serial = make_key(PREFIX_WL, issuer, serial);
         match self.read_entry(&issuer_serial) {
             Ok(Some(value)) => Ok(value),
             Ok(None) => Ok(nsICertStorage::STATE_UNSET as i16),
             Err(_) => Err(SecurityStateError::from("problem reading whitelist state")),
         }
     }
 
     pub fn is_data_fresh(
@@ -402,17 +399,17 @@ fn do_construct_cert_storage(
 }
 
 fn read_int_pref(name: &str) -> Result<i32, SecurityStateError> {
     let pref_service = match xpcom::services::get_PreferencesService() {
         Some(ps) => ps,
         _ => {
             return Err(SecurityStateError::from(
                 "could not get preferences service",
-            ))
+            ));
         }
     };
 
     let prefs: RefPtr<nsIPrefBranch> = match (*pref_service).query_interface() {
         Some(pb) => pb,
         _ => return Err(SecurityStateError::from("could not QI to nsIPrefBranch")),
     };
     let pref_name = match CString::new(name) {
@@ -424,17 +421,17 @@ fn read_int_pref(name: &str) -> Result<i
 
     // We can't use GetIntPrefWithDefault because optional_argc is not
     // supported. No matter, we can just check for failure and ignore
     // any NS_ERROR_UNEXPECTED result.
     let res = unsafe { (*prefs).GetIntPref((&pref_name).as_ptr(), (&mut pref_value) as *mut i32) };
     if !res.succeeded() {
         match res.0 {
             r if r == nsresult::NS_ERROR_UNEXPECTED as u32 => (),
-            _ => return Err(SecurityStateError::from("could not read pref")), // nserror::nsresult(err),
+            _ => return Err(SecurityStateError::from("could not read pref")),
         }
     }
     Ok(pref_value)
 }
 
 #[no_mangle]
 pub extern "C" fn cert_storage_constructor(
     outer: *const nsISupports,
@@ -449,20 +446,28 @@ pub extern "C" fn cert_storage_construct
         Ok(_) => nserror::NS_OK,
         Err(_) => {
             // In future: log something so we know what went wrong?
             nserror::NS_ERROR_FAILURE
         }
     }
 }
 
+macro_rules! try_ns {
+    ($e:expr) => {
+        match $e {
+            Ok(value) => value,
+            Err(_) => return nserror::NS_ERROR_FAILURE,
+        }
+    };
+}
+
 #[derive(xpcom)]
 #[xpimplements(nsICertStorage, nsIObserver)]
 #[refcnt = "atomic"]
-
 struct InitCertStorage {
     security_state: RwLock<SecurityState>,
 }
 
 #[allow(non_snake_case)]
 impl CertStorage {
     unsafe fn setup_prefs(&self) -> Result<(), SecurityStateError> {
         let int_prefs = [
@@ -502,215 +507,182 @@ impl CertStorage {
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
         state: i16,
     ) -> nserror::nsresult {
         if issuer.is_null() || serial.is_null() {
             return nserror::NS_ERROR_FAILURE;
         }
-        let mut ss = match self.security_state.write() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(write_guard) => write_guard,
-        };
-        match ss.set_revocation_by_issuer_and_serial(
-            (*issuer).as_str_unchecked(),
-            (*serial).as_str_unchecked(),
-            state,
-        ) {
+        let issuer_decoded = try_ns!(base64::decode(&*issuer));
+        let serial_decoded = try_ns!(base64::decode(&*serial));
+        let mut ss = try_ns!(self.security_state.write());
+        match ss.set_revocation_by_issuer_and_serial(&issuer_decoded, &serial_decoded, state) {
             Ok(_) => nserror::NS_OK,
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn SetRevocationBySubjectAndPubKey(
         &self,
         subject: *const nsACString,
         pub_key_base64: *const nsACString,
         state: i16,
     ) -> nserror::nsresult {
         if subject.is_null() || pub_key_base64.is_null() {
             return nserror::NS_ERROR_FAILURE;
         }
-        let mut ss = match self.security_state.write() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(write_guard) => write_guard,
-        };
-        match ss.set_revocation_by_subject_and_pub_key(
-            (*subject).as_str_unchecked(),
-            (*pub_key_base64).as_str_unchecked(),
-            state,
-        ) {
+        let subject_decoded = try_ns!(base64::decode(&*subject));
+        let pub_key_decoded = try_ns!(base64::decode(&*pub_key_base64));
+        let mut ss = try_ns!(self.security_state.write());
+        match ss.set_revocation_by_subject_and_pub_key(&subject_decoded, &pub_key_decoded, state) {
             Ok(_) => nserror::NS_OK,
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn SetEnrollment(
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
         state: i16,
     ) -> nserror::nsresult {
         if issuer.is_null() || serial.is_null() {
             return nserror::NS_ERROR_FAILURE;
         }
-        let mut ss = match self.security_state.write() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(write_guard) => write_guard,
-        };
-        match ss.set_enrollment(
-            (*issuer).as_str_unchecked(),
-            (*serial).as_str_unchecked(),
-            state,
-        ) {
+        let issuer_decoded = try_ns!(base64::decode(&*issuer));
+        let serial_decoded = try_ns!(base64::decode(&*serial));
+        let mut ss = try_ns!(self.security_state.write());
+        match ss.set_enrollment(&issuer_decoded, &serial_decoded, state) {
             Ok(_) => nserror::NS_OK,
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn SetWhitelist(
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
         state: i16,
     ) -> nserror::nsresult {
         if issuer.is_null() || serial.is_null() {
             return nserror::NS_ERROR_FAILURE;
         }
-        let mut ss = match self.security_state.write() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(write_guard) => write_guard,
-        };
-        match ss.set_whitelist(
-            (*issuer).as_str_unchecked(),
-            (*serial).as_str_unchecked(),
-            state,
-        ) {
+        let issuer_decoded = try_ns!(base64::decode(&*issuer));
+        let serial_decoded = try_ns!(base64::decode(&*serial));
+        let mut ss = try_ns!(self.security_state.write());
+        match ss.set_whitelist(&issuer_decoded, &serial_decoded, state) {
             Ok(_) => nserror::NS_OK,
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn GetRevocationState(
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
         subject: *const nsACString,
         pub_key_base64: *const nsACString,
         state: *mut i16,
     ) -> nserror::nsresult {
         if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key_base64.is_null() {
             return nserror::NS_ERROR_FAILURE;
         }
-
-        let ss = match self.security_state.read() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(read_guard) => read_guard,
-        };
-
+        // TODO (bug 1535752): If we're calling this function when we already have binary data (e.g.
+        // in a TrustDomain::GetCertTrust callback), we should be able to pass in the binary data
+        // directly. See also bug 1535486.
+        let issuer_decoded = try_ns!(base64::decode(&*issuer));
+        let serial_decoded = try_ns!(base64::decode(&*serial));
+        let subject_decoded = try_ns!(base64::decode(&*subject));
+        let pub_key_decoded = try_ns!(base64::decode(&*pub_key_base64));
+        let ss = try_ns!(self.security_state.read());
         *state = nsICertStorage::STATE_UNSET as i16;
         match ss.get_revocation_state(
-            (*issuer).as_str_unchecked(),
-            (*serial).as_str_unchecked(),
-            (*subject).as_str_unchecked(),
-            (*pub_key_base64).as_str_unchecked(),
+            &issuer_decoded,
+            &serial_decoded,
+            &subject_decoded,
+            &pub_key_decoded,
         ) {
             Ok(st) => {
                 *state = st;
                 nserror::NS_OK
             }
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn GetEnrollmentState(
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
         state: *mut i16,
     ) -> nserror::nsresult {
-        let ss = match self.security_state.read() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(read_guard) => read_guard,
-        };
-
+        if issuer.is_null() || serial.is_null() {
+            return nserror::NS_ERROR_FAILURE;
+        }
+        let issuer_decoded = try_ns!(base64::decode(&*issuer));
+        let serial_decoded = try_ns!(base64::decode(&*serial));
+        let ss = try_ns!(self.security_state.read());
         *state = nsICertStorage::STATE_UNSET as i16;
-
-        match ss.get_enrollment_state((*issuer).as_str_unchecked(), (*serial).as_str_unchecked()) {
+        match ss.get_enrollment_state(&issuer_decoded, &serial_decoded) {
             Ok(st) => {
                 *state = st;
                 nserror::NS_OK
             }
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn GetWhitelistState(
         &self,
         issuer: *const nsACString,
         serial: *const nsACString,
         state: *mut i16,
     ) -> nserror::nsresult {
-        let ss = match self.security_state.read() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(read_guard) => read_guard,
-        };
-
+        if issuer.is_null() || serial.is_null() {
+            return nserror::NS_ERROR_FAILURE;
+        }
+        let issuer_decoded = try_ns!(base64::decode(&*issuer));
+        let serial_decoded = try_ns!(base64::decode(&*serial));
+        let ss = try_ns!(self.security_state.read());
         *state = nsICertStorage::STATE_UNSET as i16;
-
-        match ss.get_whitelist_state((*issuer).as_str_unchecked(), (*serial).as_str_unchecked()) {
+        match ss.get_whitelist_state(&issuer_decoded, &serial_decoded) {
             Ok(st) => {
                 *state = st;
                 nserror::NS_OK
             }
             _ => nserror::NS_ERROR_FAILURE,
         }
     }
 
     unsafe fn IsBlocklistFresh(&self, fresh: *mut bool) -> nserror::nsresult {
         *fresh = false;
-
-        let ss = match self.security_state.read() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(read_guard) => read_guard,
-        };
-
+        let ss = try_ns!(self.security_state.read());
         *fresh = match ss.is_blocklist_fresh() {
             Ok(is_fresh) => is_fresh,
             Err(_) => false,
         };
 
         nserror::NS_OK
     }
 
     unsafe fn IsWhitelistFresh(&self, fresh: *mut bool) -> nserror::nsresult {
         *fresh = false;
-
-        let ss = match self.security_state.read() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(read_guard) => read_guard,
-        };
-
+        let ss = try_ns!(self.security_state.read());
         *fresh = match ss.is_whitelist_fresh() {
             Ok(is_fresh) => is_fresh,
             Err(_) => false,
         };
 
         nserror::NS_OK
     }
 
     unsafe fn IsEnrollmentFresh(&self, fresh: *mut bool) -> nserror::nsresult {
         *fresh = false;
-
-        let ss = match self.security_state.read() {
-            Err(_) => return nserror::NS_ERROR_FAILURE,
-            Ok(read_guard) => read_guard,
-        };
-
+        let ss = try_ns!(self.security_state.read());
         *fresh = match ss.is_enrollment_fresh() {
             Ok(is_fresh) => is_fresh,
             Err(_) => false,
         };
 
         nserror::NS_OK
     }
 
@@ -739,28 +711,22 @@ impl CertStorage {
                 let mut name_string = nsCString::new();
                 name_string.assign_utf16_to_utf8(name_slice);
 
                 let pref_name = match CString::new(name_string.as_str_unchecked()) {
                     Ok(n) => n,
                     _ => return nserror::NS_ERROR_FAILURE,
                 };
 
-                let res = prefs.GetIntPref(
-                    (&pref_name).as_ptr(),
-                    (&mut pref_value) as *mut i32,
-                );
+                let res = prefs.GetIntPref((&pref_name).as_ptr(), (&mut pref_value) as *mut i32);
 
                 if !res.succeeded() {
                     return res;
                 }
 
-                let mut ss = match self.security_state.write() {
-                    Err(_) => return nserror::NS_ERROR_FAILURE,
-                    Ok(write_guard) => write_guard,
-                };
+                let mut ss = try_ns!(self.security_state.write());
                 ss.pref_seen(name_string.as_str_unchecked(), pref_value);
             }
             _ => (),
         }
         nserror::NS_OK
     }
 }