Bug 1538093 - reopen security_state env as read-only when not writing r=keeler
authorMyk Melez <myk@mykzilla.org>
Fri, 29 Mar 2019 19:48:00 +0000
changeset 466844 3561cdc13806a6ce06b42417acaef8c73c847681
parent 466843 6c34f19750b8ec4b39bf616c557914f6f932ce2f
child 466845 b9eba27b9cf664c30de177ef19da91361130c17e
push id112603
push usernerli@mozilla.com
push dateSat, 30 Mar 2019 09:35:57 +0000
treeherdermozilla-inbound@7c3183c56eb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1538093
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 1538093 - reopen security_state env as read-only when not writing r=keeler The new rkv-based cert_storage database caused a Heap Unclassified regression because of memory that LMDB reserves when opening a database in read-write mode. Since cert_storage usage is read-heavy, this change claws back that regression by opening it in read-only mode except when changes are being made. Differential Revision: https://phabricator.services.mozilla.com/D25098
Cargo.lock
security/manager/ssl/cert_storage/Cargo.toml
security/manager/ssl/cert_storage/src/lib.rs
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -429,16 +429,17 @@ name = "cc"
 version = "1.0.23"
 source = "git+https://github.com/glandium/cc-rs?branch=1.0.23-clang-cl-aarch64#2aa71628b1261b5515bd8668afca591669ba195d"
 
 [[package]]
 name = "cert_storage"
 version = "0.0.1"
 dependencies = [
  "base64 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "lmdb-rkv 0.11.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "nserror 0.1.0",
  "nsstring 0.1.0",
  "rkv 0.9.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "sha2 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "style 0.0.1",
  "time 0.1.40 (registry+https://github.com/rust-lang/crates.io-index)",
  "xpcom 0.1.0",
 ]
--- a/security/manager/ssl/cert_storage/Cargo.toml
+++ b/security/manager/ssl/cert_storage/Cargo.toml
@@ -1,14 +1,15 @@
 [package]
 name = "cert_storage"
 version = "0.0.1"
 authors = ["Dana Keeler <dkeeler@mozilla.com>", "Mark Goodwin <mgoodwin@mozilla.com"]
 
 [dependencies]
 base64 = "0.10"
+lmdb-rkv = "0.11"
 nserror = { path = "../../../../xpcom/rust/nserror" }
 nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rkv = "^0.9"
 sha2 = "^0.7"
 style = { path = "../../../../servo/components/style" }
 time = "0.1"
 xpcom = { path = "../../../../xpcom/rust/xpcom" }
--- a/security/manager/ssl/cert_storage/src/lib.rs
+++ b/security/manager/ssl/cert_storage/src/lib.rs
@@ -1,13 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 extern crate base64;
+extern crate lmdb;
 extern crate nserror;
 extern crate nsstring;
 extern crate rkv;
 extern crate sha2;
 extern crate time;
 #[macro_use]
 extern crate xpcom;
 extern crate style;
@@ -24,16 +25,17 @@ 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 lmdb::EnvironmentFlags;
 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> {
@@ -88,39 +90,43 @@ impl SecurityState {
     pub fn db_needs_opening(&self) -> bool {
         self.env_and_store.is_none()
     }
 
     pub fn open_db(&mut self) -> Result<(), SecurityStateError> {
         if self.env_and_store.is_some() {
             return Ok(());
         }
-        let mut store_path = self.profile_path.clone();
-        store_path.push("security_state");
+
+        let store_path = get_store_path(&self.profile_path)?;
 
-        create_dir_all(store_path.as_path())?;
+        // Open the store in read-write mode initially to create it (if needed)
+        // and migrate data from the old store (if any).
         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 store = env.open_single("cert_storage", StoreOptions::create())?;
 
         // if the profile has a revocations.txt, migrate it and remove the file
         let mut revocations_path = self.profile_path.clone();
         revocations_path.push("revocations.txt");
         if revocations_path.exists() {
             SecurityState::migrate(&revocations_path, &env, &store)?;
             remove_file(revocations_path)?;
         }
+
         // We already returned early if env_and_store was Some, so this should take the None branch.
         match self.env_and_store.replace(EnvAndStore { env, store }) {
             Some(_) => Err(SecurityStateError::from(
                 "env and store already initialized? (did we mess up our threading model?)",
             )),
             None => Ok(()),
-        }
+        }?;
+
+        // Now reopen the store in read-only mode to conserve memory.
+        // We'll reopen it again in read-write mode when making changes.
+        self.reopen_store_read_only()
     }
 
     fn migrate(
         revocations_path: &PathBuf,
         env: &Rkv,
         store: &SingleStore,
     ) -> Result<(), SecurityStateError> {
         let f = File::open(revocations_path)?;
@@ -175,26 +181,60 @@ impl SecurityState {
                 }
             }
         }
 
         writer.commit()?;
         Ok(())
     }
 
+    fn reopen_store_read_write(&mut self) -> Result<(), SecurityStateError> {
+        let store_path = get_store_path(&self.profile_path)?;
+
+        // Move out and drop the EnvAndStore first so we don't have
+        // two LMDB environments open at the same time.
+        drop(self.env_and_store.take());
+
+        let env = Rkv::new(store_path.as_path())?;
+        let store = env.open_single("cert_storage", StoreOptions::create())?;
+        self.env_and_store.replace(EnvAndStore { env, store });
+        Ok(())
+    }
+
+    fn reopen_store_read_only(&mut self) -> Result<(), SecurityStateError> {
+        let store_path = get_store_path(&self.profile_path)?;
+
+        // Move out and drop the EnvAndStore first so we don't have
+        // two LMDB environments open at the same time.
+        drop(self.env_and_store.take());
+
+        let mut builder = Rkv::environment_builder();
+        builder.set_max_dbs(2);
+        builder.set_flags(EnvironmentFlags::READ_ONLY);
+
+        let env = Rkv::from_env(store_path.as_path(), builder)?;
+        let store = env.open_single("cert_storage", StoreOptions::default())?;
+        self.env_and_store.replace(EnvAndStore { env, store });
+        Ok(())
+    }
+
     fn write_entry(&mut self, key: &[u8], value: i16) -> Result<(), SecurityStateError> {
-        let env_and_store = match self.env_and_store.as_mut() {
-            Some(env_and_store) => env_and_store,
-            None => return Err(SecurityStateError::from("env and store not initialized?")),
-        };
-        let mut writer = env_and_store.env.write()?;
-        env_and_store
-            .store
-            .put(&mut writer, key, &Value::I64(value as i64))?;
-        writer.commit()?;
+        self.reopen_store_read_write()?;
+        {
+            let env_and_store = match self.env_and_store.as_mut() {
+                Some(env_and_store) => env_and_store,
+                None => return Err(SecurityStateError::from("env and store not initialized?")),
+            };
+            let mut writer = env_and_store.env.write()?;
+            env_and_store
+                .store
+                .put(&mut writer, key, &Value::I64(value as i64))?;
+            writer.commit()?;
+        }
+        self.reopen_store_read_only()?;
         Ok(())
     }
 
     fn read_entry(&self, key: &[u8]) -> Result<Option<i16>, SecurityStateError> {
         let env_and_store = match self.env_and_store.as_ref() {
             Some(env_and_store) => env_and_store,
             None => return Err(SecurityStateError::from("env and store not initialized?")),
         };
@@ -401,28 +441,33 @@ fn get_path_from_directory_service(key: 
             .map_err(|res| SecurityStateError {
                 message: (*res.error_name()).as_str_unchecked().to_owned(),
             })?;
     }
 
     Ok(PathBuf::from(format!("{}", path)))
 }
 
+fn get_profile_path() -> Result<PathBuf, SecurityStateError> {
+    Ok(get_path_from_directory_service("ProfD").or_else(|_| get_path_from_directory_service("TmpD"))?)
+}
+
+fn get_store_path(profile_path: &PathBuf) -> Result<PathBuf, SecurityStateError> {
+    let mut store_path = profile_path.clone();
+    store_path.push("security_state");
+    create_dir_all(store_path.as_path())?;
+    Ok(store_path)
+}
+
 fn do_construct_cert_storage(
     _outer: *const nsISupports,
     iid: *const xpcom::nsIID,
     result: *mut *mut xpcom::reexports::libc::c_void,
 ) -> Result<(), SecurityStateError> {
-    let path_buf = match get_path_from_directory_service("ProfD") {
-        Ok(path) => path,
-        Err(_) => match get_path_from_directory_service("TmpD") {
-            Ok(path) => path,
-            Err(e) => return Err(e),
-        },
-    };
+    let path_buf = get_profile_path()?;
 
     let cert_storage = CertStorage::allocate(InitCertStorage {
         security_state: RwLock::new(SecurityState::new(path_buf)?),
     });
 
     unsafe {
         cert_storage
             .QueryInterface(iid, result)