aconfig: update aconfig_storage_read_api

Previously we are ensuring that the public rust api to get mapped file
is safe as we are ensuring that the file being mapped has a permission
of 444. However, a file permission of 444 is not possible due to build
system needs to make file 644 while creating img files. Thus need to make the rust api to get mapped file unsafe.

In reality, this should have no impact. Because, even though the storage
files have a file permission of 644, they are on a RO partition like
system. So the files are not writable anyway. This is true for all the
containers (platform partitions and mainline modules) we know so far.

Bug: b/321077378
Test: atest aconfig_storage_read_api.test; atest
aconfig_storage_read_api.test.rust; atest
aconfig_storage_read_api.test.cpp

Change-Id: I643fe191e697a524e2303a32750f04c268f408fd

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),