Merge "aconfig: update aconfig_storage_read_api" into main

6 files changed, 83 insertions, 135 deletions

diff --git a/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp b/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp
index b5bcf7d4c5..ea756b3962 100644
--- a/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp
+++ b/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp
@@ -74,11 +74,6 @@ static Result<MappedStorageFile> map_storage_file(std::string const& file) {
   if (fstat(fd, &fd_stat) < 0) {
     return Error() << "fstat failed";
   }
-
-  if ((fd_stat.st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0) {
-    return Error() << "cannot map writeable file";
-  }
-
   size_t file_size = fd_stat.st_size;
 
   void* const map_result = mmap(nullptr, file_size, PROT_READ, MAP_SHARED, fd, 0);
diff --git a/tools/aconfig/aconfig_storage_read_api/src/lib.rs b/tools/aconfig/aconfig_storage_read_api/src/lib.rs
index ea45f5d464..8a71480134 100644
--- a/tools/aconfig/aconfig_storage_read_api/src/lib.rs
+++ b/tools/aconfig/aconfig_storage_read_api/src/lib.rs
@@ -64,11 +64,17 @@ pub const STORAGE_LOCATION_FILE: &str = "/metadata/aconfig/boot/available_storag
 /// \input container: the flag package container
 /// \input file_type: stoarge file type enum
 /// \return a result of read only mapped file
-pub fn get_mapped_storage_file(
+///
+/// # Safety
+///
+/// The memory mapped file may have undefined behavior if there are writes to this
+/// file after being mapped. Ensure no writes can happen to this file while this
+/// mapping stays alive.
+pub unsafe fn get_mapped_storage_file(
     container: &str,
     file_type: StorageFileType,
 ) -> Result<Mmap, AconfigStorageError> {
-    crate::mapped_file::get_mapped_file(STORAGE_LOCATION_FILE, container, file_type)
+    unsafe { crate::mapped_file::get_mapped_file(STORAGE_LOCATION_FILE, container, file_type) }
 }
 
 /// Get package start offset for flags.
@@ -311,10 +317,10 @@ mod tests {
     use aconfig_storage_file::protos::storage_record_pb::write_proto_to_temp_file;
     use tempfile::NamedTempFile;
 
-    fn create_test_storage_files(read_only: bool) -> [NamedTempFile; 4] {
-        let package_map = copy_to_temp_file("./tests/package.map", read_only).unwrap();
-        let flag_map = copy_to_temp_file("./tests/flag.map", read_only).unwrap();
-        let flag_val = copy_to_temp_file("./tests/flag.val", read_only).unwrap();
+    fn create_test_storage_files() -> [NamedTempFile; 4] {
+        let package_map = copy_to_temp_file("./tests/package.map").unwrap();
+        let flag_map = copy_to_temp_file("./tests/flag.map").unwrap();
+        let flag_val = copy_to_temp_file("./tests/flag.val").unwrap();
 
         let text_proto = format!(
             r#"
@@ -338,10 +344,11 @@ files {{
     #[test]
     // this test point locks down flag package offset query
     fn test_package_offset_query() {
-        let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files(true);
+        let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
-        let package_mapped_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap();
+        let package_mapped_file = unsafe {
+            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap()
+        };
 
         let package_offset =
             get_package_offset(&package_mapped_file, "com.android.aconfig.storage.test_1")
@@ -368,10 +375,10 @@ files {{
     #[test]
     // this test point locks down flag offset query
     fn test_flag_offset_query() {
-        let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files(true);
+        let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
         let flag_mapped_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap();
+            unsafe { get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap() };
 
         let baseline = vec![
             (0, "enabled_ro", 1u16),
@@ -393,10 +400,10 @@ files {{
     #[test]
     // this test point locks down flag offset query
     fn test_flag_value_query() {
-        let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files(true);
+        let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
         let flag_value_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap();
+            unsafe { get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap() };
         let baseline: Vec<bool> = vec![false; 8];
         for (offset, expected_value) in baseline.into_iter().enumerate() {
             let flag_value = get_boolean_flag_value(&flag_value_file, offset as u32).unwrap();
@@ -407,7 +414,6 @@ files {{
     #[test]
     // this test point locks down flag storage file version number query api
     fn test_storage_version_query() {
-        let _ro_files = create_test_storage_files(true);
         assert_eq!(get_storage_file_version("./tests/package.map").unwrap(), 1);
         assert_eq!(get_storage_file_version("./tests/flag.map").unwrap(), 1);
         assert_eq!(get_storage_file_version("./tests/flag.val").unwrap(), 1);
diff --git a/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs b/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs
index 09ecdb643d..86c6a1b053 100644
--- a/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs
+++ b/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-use std::fs::{self, File};
+use std::fs::File;
 use std::io::{BufReader, Read};
 
 use anyhow::anyhow;
@@ -56,26 +56,16 @@ fn find_container_storage_location(
     Err(StorageFileNotFound(anyhow!("Storage file does not exist for {}", container)))
 }
 
-/// Verify the file is read only and then map it
-fn verify_read_only_and_map(file_path: &str) -> Result<Mmap, AconfigStorageError> {
-    // ensure file has read only permission
-    let perms = fs::metadata(file_path).unwrap().permissions();
-    if !perms.readonly() {
-        return Err(MapFileFail(anyhow!("fail to map non read only storage file {}", file_path)));
-    }
-
+/// Get the read only memory mapping of a storage file
+///
+/// # Safety
+///
+/// The memory mapped file may have undefined behavior if there are writes to this
+/// file after being mapped. Ensure no writes can happen to this file while this
+/// mapping stays alive.
+unsafe fn map_file(file_path: &str) -> Result<Mmap, AconfigStorageError> {
     let file = File::open(file_path)
         .map_err(|errmsg| FileReadFail(anyhow!("Failed to open file {}: {}", file_path, errmsg)))?;
-
-    // SAFETY:
-    //
-    // Mmap constructors are unsafe as it would have undefined behaviors if the file
-    // is modified after mapped (https://docs.rs/memmap2/latest/memmap2/struct.Mmap.html).
-    //
-    // We either have to make this api unsafe or ensure that the file will not be modified
-    // which means it is read only. Here in the code, we check explicitly that the file
-    // being mapped must only have read permission, otherwise, error out, thus making sure
-    // it is safe.
     unsafe {
         let mapped_file = Mmap::map(&file).map_err(|errmsg| {
             MapFileFail(anyhow!("fail to map storage file {}: {}", file_path, errmsg))
@@ -85,16 +75,22 @@ fn verify_read_only_and_map(file_path: &str) -> Result<Mmap, AconfigStorageError
 }
 
 /// Get a mapped storage file given the container and file type
-pub fn get_mapped_file(
+///
+/// # Safety
+///
+/// The memory mapped file may have undefined behavior if there are writes to this
+/// file after being mapped. Ensure no writes can happen to this file while this
+/// mapping stays alive.
+pub unsafe fn get_mapped_file(
     location_pb_file: &str,
     container: &str,
     file_type: StorageFileType,
 ) -> Result<Mmap, AconfigStorageError> {
     let files_location = find_container_storage_location(location_pb_file, container)?;
     match file_type {
-        StorageFileType::PackageMap => verify_read_only_and_map(files_location.package_map()),
-        StorageFileType::FlagMap => verify_read_only_and_map(files_location.flag_map()),
-        StorageFileType::FlagVal => verify_read_only_and_map(files_location.flag_val()),
+        StorageFileType::PackageMap => unsafe { map_file(files_location.package_map()) },
+        StorageFileType::FlagMap => unsafe { map_file(files_location.flag_map()) },
+        StorageFileType::FlagVal => unsafe { map_file(files_location.flag_val()) },
     }
 }
 
@@ -155,14 +151,15 @@ files {
         let mut content = Vec::new();
         opened_file.read_to_end(&mut content).unwrap();
 
-        let mmaped_file = get_mapped_file(location_pb_file, "system", file_type).unwrap();
+        let mmaped_file =
+            unsafe { get_mapped_file(location_pb_file, "system", file_type).unwrap() };
         assert_eq!(mmaped_file[..], content[..]);
     }
 
-    fn create_test_storage_files(read_only: bool) -> [NamedTempFile; 4] {
-        let package_map = copy_to_temp_file("./tests/package.map", read_only).unwrap();
-        let flag_map = copy_to_temp_file("./tests/flag.map", read_only).unwrap();
-        let flag_val = copy_to_temp_file("./tests/package.map", read_only).unwrap();
+    fn create_test_storage_files() -> [NamedTempFile; 4] {
+        let package_map = copy_to_temp_file("./tests/package.map").unwrap();
+        let flag_map = copy_to_temp_file("./tests/flag.map").unwrap();
+        let flag_val = copy_to_temp_file("./tests/package.map").unwrap();
 
         let text_proto = format!(
             r#"
@@ -185,7 +182,7 @@ files {{
 
     #[test]
     fn test_mapped_file_contents() {
-        let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files(true);
+        let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
         map_and_verify(
             &pb_file_path,
@@ -203,35 +200,4 @@ files {{
             &flag_val.path().display().to_string(),
         );
     }
-
-    #[test]
-    fn test_map_non_read_only_file() {
-        let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files(false);
-        let pb_file_path = pb_file.path().display().to_string();
-        let error =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap_err();
-        assert_eq!(
-            format!("{:?}", error),
-            format!(
-                "MapFileFail(fail to map non read only storage file {})",
-                package_map.path().display()
-            )
-        );
-        let error = get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap_err();
-        assert_eq!(
-            format!("{:?}", error),
-            format!(
-                "MapFileFail(fail to map non read only storage file {})",
-                flag_map.path().display()
-            )
-        );
-        let error = get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap_err();
-        assert_eq!(
-            format!("{:?}", error),
-            format!(
-                "MapFileFail(fail to map non read only storage file {})",
-                flag_val.path().display()
-            )
-        );
-    }
 }
diff --git a/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs b/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs
index ff724990a9..84f31aa710 100644
--- a/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs
+++ b/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs
@@ -19,14 +19,8 @@ use std::fs;
 use tempfile::NamedTempFile;
 
 /// Create temp file copy
-pub(crate) fn copy_to_temp_file(source_file: &str, read_only: bool) -> Result<NamedTempFile> {
+pub(crate) fn copy_to_temp_file(source_file: &str) -> Result<NamedTempFile> {
     let file = NamedTempFile::new()?;
     fs::copy(source_file, file.path())?;
-    if read_only {
-        let file_name = file.path().display().to_string();
-        let mut perms = fs::metadata(file_name).unwrap().permissions();
-        perms.set_readonly(true);
-        fs::set_permissions(file.path(), perms.clone()).unwrap();
-    }
     Ok(file)
 }
diff --git a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp
index 377395a419..1d36aae8cb 100644
--- a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp
+++ b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp
@@ -33,7 +33,7 @@ namespace private_api = aconfig_storage::private_internal_api;
 
 class AconfigStorageTest : public ::testing::Test {
  protected:
-  Result<std::string> copy_to_ro_temp_file(std::string const& source_file) {
+  Result<std::string> copy_to_temp_file(std::string const& source_file) {
     auto temp_file = std::string(std::tmpnam(nullptr));
     auto content = std::string();
     if (!ReadFileToString(source_file, &content)) {
@@ -42,9 +42,6 @@ class AconfigStorageTest : public ::testing::Test {
     if (!WriteStringToFile(content, temp_file)) {
       return Error() << "failed to copy file: " << source_file;
     }
-    if (chmod(temp_file.c_str(), S_IRUSR | S_IRGRP | S_IROTH) == -1) {
-      return Error() << "failed to make file read only";
-    }
     return temp_file;
   }
 
@@ -71,9 +68,9 @@ class AconfigStorageTest : public ::testing::Test {
 
   void SetUp() override {
     auto const test_dir = android::base::GetExecutableDirectory();
-    package_map = *copy_to_ro_temp_file(test_dir + "/package.map");
-    flag_map = *copy_to_ro_temp_file(test_dir + "/flag.map");
-    flag_val = *copy_to_ro_temp_file(test_dir + "/flag.val");
+    package_map = *copy_to_temp_file(test_dir + "/package.map");
+    flag_map = *copy_to_temp_file(test_dir + "/flag.map");
+    flag_val = *copy_to_temp_file(test_dir + "/flag.val");
     storage_record_pb = *write_storage_location_pb_file(
         package_map, flag_map, flag_val);
   }
@@ -113,27 +110,6 @@ TEST_F(AconfigStorageTest, test_none_exist_storage_file_mapping) {
             "Unable to find storage files for container vendor");
 }
 
-/// Negative test to lock down the error when mapping a writeable storage file
-TEST_F(AconfigStorageTest, test_writable_storage_file_mapping) {
-  ASSERT_TRUE(chmod(package_map.c_str(), 0666) != -1);
-  auto mapped_file = private_api::get_mapped_file_impl(
-      storage_record_pb, "system", api::StorageFileType::package_map);
-  ASSERT_FALSE(mapped_file.ok());
-  ASSERT_EQ(mapped_file.error().message(), "cannot map writeable file");
-
-  ASSERT_TRUE(chmod(flag_map.c_str(), 0666) != -1);
-  mapped_file = private_api::get_mapped_file_impl(
-      storage_record_pb, "system", api::StorageFileType::flag_map);
-  ASSERT_FALSE(mapped_file.ok());
-  ASSERT_EQ(mapped_file.error().message(), "cannot map writeable file");
-
-  ASSERT_TRUE(chmod(flag_val.c_str(), 0666) != -1);
-  mapped_file = private_api::get_mapped_file_impl(
-      storage_record_pb, "system", api::StorageFileType::flag_val);
-  ASSERT_FALSE(mapped_file.ok());
-  ASSERT_EQ(mapped_file.error().message(), "cannot map writeable file");
-}
-
 /// Test to lock down storage package offset query api
 TEST_F(AconfigStorageTest, test_package_offset_query) {
   auto mapped_file = private_api::get_mapped_file_impl(
diff --git a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs
index eb4d54d77a..afcd5a7f3c 100644
--- a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs
+++ b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs
@@ -9,20 +9,16 @@ mod aconfig_storage_rust_test {
     use std::fs;
     use tempfile::NamedTempFile;
 
-    pub fn copy_to_ro_temp_file(source_file: &str) -> NamedTempFile {
+    pub fn copy_to_temp_file(source_file: &str) -> NamedTempFile {
         let file = NamedTempFile::new().unwrap();
         fs::copy(source_file, file.path()).unwrap();
-        let file_name = file.path().display().to_string();
-        let mut perms = fs::metadata(file_name).unwrap().permissions();
-        perms.set_readonly(true);
-        fs::set_permissions(file.path(), perms.clone()).unwrap();
         file
     }
 
     fn create_test_storage_files() -> [NamedTempFile; 4] {
-        let package_map = copy_to_ro_temp_file("./package.map");
-        let flag_map = copy_to_ro_temp_file("./flag.map");
-        let flag_val = copy_to_ro_temp_file("./flag.val");
+        let package_map = copy_to_temp_file("./package.map");
+        let flag_map = copy_to_temp_file("./flag.map");
+        let flag_val = copy_to_temp_file("./flag.val");
 
         let text_proto = format!(
             r#"
@@ -47,8 +43,11 @@ files {{
     fn test_unavailable_stoarge() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
-        let err =
-            get_mapped_file(&pb_file_path, "vendor", StorageFileType::PackageMap).unwrap_err();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
+        let err = unsafe {
+            get_mapped_file(&pb_file_path, "vendor", StorageFileType::PackageMap).unwrap_err()
+        };
         assert_eq!(
             format!("{:?}", err),
             "StorageFileNotFound(Storage file does not exist for vendor)"
@@ -59,8 +58,11 @@ files {{
     fn test_package_offset_query() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
-        let package_mapped_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
+        let package_mapped_file = unsafe {
+            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap()
+        };
 
         let package_offset =
             get_package_offset(&package_mapped_file, "com.android.aconfig.storage.test_1")
@@ -88,8 +90,12 @@ files {{
     fn test_none_exist_package_offset_query() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
-        let package_mapped_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
+        let package_mapped_file = unsafe {
+            get_mapped_file(&pb_file_path, "system", StorageFileType::PackageMap).unwrap()
+        };
+
         let package_offset_option =
             get_package_offset(&package_mapped_file, "com.android.aconfig.storage.test_3").unwrap();
         assert_eq!(package_offset_option, None);
@@ -99,8 +105,10 @@ files {{
     fn test_flag_offset_query() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
         let flag_mapped_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap();
+            unsafe { get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap() };
 
         let baseline = vec![
             (0, "enabled_ro", 1u16),
@@ -123,9 +131,10 @@ files {{
     fn test_none_exist_flag_offset_query() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
         let flag_mapped_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap();
-
+            unsafe { get_mapped_file(&pb_file_path, "system", StorageFileType::FlagMap).unwrap() };
         let flag_offset_option = get_flag_offset(&flag_mapped_file, 0, "none_exist").unwrap();
         assert_eq!(flag_offset_option, None);
 
@@ -137,9 +146,10 @@ files {{
     fn test_boolean_flag_value_query() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
         let flag_value_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap();
-
+            unsafe { get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap() };
         let baseline: Vec<bool> = vec![false; 8];
         for (offset, expected_value) in baseline.into_iter().enumerate() {
             let flag_value = get_boolean_flag_value(&flag_value_file, offset as u32).unwrap();
@@ -151,9 +161,10 @@ files {{
     fn test_invalid_boolean_flag_value_query() {
         let [_package_map, _flag_map, _flag_val, pb_file] = create_test_storage_files();
         let pb_file_path = pb_file.path().display().to_string();
+        // SAFETY:
+        // The safety here is ensured as the test process will not write to temp storage file
         let flag_value_file =
-            get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap();
-
+            unsafe { get_mapped_file(&pb_file_path, "system", StorageFileType::FlagVal).unwrap() };
         let err = get_boolean_flag_value(&flag_value_file, 8u32).unwrap_err();
         assert_eq!(
             format!("{:?}", err),