StrixOS/build
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

aconfig: update aconfig_storage_read_api

Browse Source 
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
This commit is contained in:
Dennis Shen
2024-03-21 01:36:41 +00:00
parent 497f02d8c0
commit bb13bbb086
6 changed files with 83 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp
View File

@@ -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);

34
tools/aconfig/aconfig_storage_read_api/src/lib.rs
View File

@@ -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);

86
tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs
View File

@@ -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()
)
);
}
}

8
tools/aconfig/aconfig_storage_read_api/src/test_utils.rs
View File

@@ -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)
}

32
tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp
View File

@@ -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(

53
tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs
View File

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