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
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 b5bcf7d..ea756b3 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 @@
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 ea45f5d..8a71480 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 @@
/// \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 @@
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 @@
#[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 @@
#[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 @@
#[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 @@
#[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 09ecdb6..86c6a1b 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 @@
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 @@
}
/// 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 @@
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 @@
#[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 @@
&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 ff72499..84f31aa 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 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 377395a..1d36aae 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 @@
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 @@
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 @@
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 @@
"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 eb4d54d..afcd5a7 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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),