commit a3e611ae14ec906d6565a00e0c640f151e501b1a
author Seth Moore <sethmo@google.com> Tue May 11 10:07:45 2021 -0700
committer Seth Moore <sethmo@google.com> Tue May 11 11:56:30 2021 -0700
tree b548e1ebb287a0a3a79041ac1e14edf98dcc0417
parent 7708000c11b2c6ffbbbd7b170b5b54ba544dbbdc

Use a RwLock for DB_PATH

The return value of DB_PATH.lock() was being borrowed, which holds the
lock for the duration of the borrow.
This is not itself a major problem, but if anything else blocked DB
object initialization, other threads could be blocked for a long time
until initialization completes.

Bug: 184006658
Test: KeyStoreTest
Change-Id: I585b40b8770b90fe80d6591157525eed0b5124c3

keystore2/src/globals.rs

diff --git a/keystore2/src/globals.rs b/keystore2/src/globals.rs
index 8d39b22..28576f6 100644
--- a/keystore2/src/globals.rs
+++ b/keystore2/src/globals.rs
@@ -39,7 +39,7 @@
 use binder::FromIBinder;
 use keystore2_vintf::get_aidl_instances;
 use lazy_static::lazy_static;
-use std::sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex, RwLock};
 use std::{cell::RefCell, sync::Once};
 use std::{collections::HashMap, path::Path, path::PathBuf};
 
@@ -55,7 +55,7 @@
 /// database connection is created for the garbage collector worker.
 pub fn create_thread_local_db() -> KeystoreDB {
     let mut db = KeystoreDB::new(
-        &DB_PATH.lock().expect("Could not get the database directory."),
+        &DB_PATH.read().expect("Could not get the database directory."),
         Some(GC.clone()),
     )
     .expect("Failed to open database.");
@@ -139,7 +139,7 @@
 
 lazy_static! {
     /// The path where keystore stores all its keys.
-    pub static ref DB_PATH: Mutex<PathBuf> = Mutex::new(
+    pub static ref DB_PATH: RwLock<PathBuf> = RwLock::new(
         Path::new("/data/misc/keystore").to_path_buf());
     /// Runtime database of unwrapped super keys.
     pub static ref SUPER_KEY: Arc<SuperKeyManager> = Default::default();
@@ -157,7 +157,7 @@
     /// LegacyBlobLoader is initialized and exists globally.
     /// The same directory used by the database is used by the LegacyBlobLoader as well.
     pub static ref LEGACY_BLOB_LOADER: Arc<LegacyBlobLoader> = Arc::new(LegacyBlobLoader::new(
-        &DB_PATH.lock().expect("Could not get the database path for legacy blob loader.")));
+        &DB_PATH.read().expect("Could not get the database path for legacy blob loader.")));
     /// Legacy migrator. Atomically migrates legacy blobs to the database.
     pub static ref LEGACY_MIGRATOR: Arc<LegacyMigrator> =
         Arc::new(LegacyMigrator::new(Arc::new(Default::default())));
@@ -173,7 +173,7 @@
                 map_km_error(km_dev.deleteKey(&*blob))
                     .context("In invalidate key closure: Trying to invalidate key blob.")
             }),
-            KeystoreDB::new(&DB_PATH.lock().expect("Could not get the database directory."), None)
+            KeystoreDB::new(&DB_PATH.read().expect("Could not get the database directory."), None)
                 .expect("Failed to open database."),
             SUPER_KEY.clone(),
         )

keystore2/src/keystore2_main.rs

diff --git a/keystore2/src/keystore2_main.rs b/keystore2/src/keystore2_main.rs
index 29330a6..53461da 100644
--- a/keystore2/src/keystore2_main.rs
+++ b/keystore2/src/keystore2_main.rs
@@ -56,7 +56,7 @@
     // For the ground truth check the service startup rule for init (typically in keystore2.rc).
     let id_rotation_state = if let Some(dir) = args.next() {
         let db_path = Path::new(&dir);
-        *keystore2::globals::DB_PATH.lock().expect("Could not lock DB_PATH.") =
+        *keystore2::globals::DB_PATH.write().expect("Could not lock DB_PATH.") =
             db_path.to_path_buf();
         IdRotationState::new(&db_path)
     } else {
@@ -121,7 +121,7 @@
     }
 
     let vpnprofilestore = VpnProfileStore::new_native_binder(
-        &keystore2::globals::DB_PATH.lock().expect("Could not get DB_PATH."),
+        &keystore2::globals::DB_PATH.read().expect("Could not get DB_PATH."),
     );
     binder::add_service(VPNPROFILESTORE_SERVICE_NAME, vpnprofilestore.as_binder()).unwrap_or_else(
         |e| {