From 00cf045c85263309324ebf74807db5ccc8748fd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Fri, 26 May 2023 16:48:01 +0200 Subject: [PATCH] aconfig: restrict valid namespace and flag names The namespace and flag names will be used as identifiers in the auto-generated code. Place restrictions on what constitutes a valid name. Valid identifiers are those that match /[a-z][a-z0-9_]/. aconfig explicitly does not implement any automatic translation to make names valid identifiers: this sidesteps potential conflicts such as "foo.bar" and "foo_bar" mapping to the same name if dots were translated to underscores. Bug: b/284252015 Test: atest aconfig.test Change-Id: I38d005a74311e5829e540063404d1565071e6e96 --- tools/aconfig/src/cache.rs | 19 ++++++------ tools/aconfig/src/codegen.rs | 43 +++++++++++++++++++++++++++ tools/aconfig/src/codegen_java.rs | 10 +++---- tools/aconfig/src/codegen_rust.rs | 12 ++++---- tools/aconfig/src/main.rs | 1 + tools/aconfig/templates/java.template | 4 +-- tools/aconfig/testdata/first.values | 6 ++-- tools/aconfig/testdata/second.values | 2 +- tools/aconfig/testdata/test.aconfig | 8 ++--- 9 files changed, 75 insertions(+), 30 deletions(-) create mode 100644 tools/aconfig/src/codegen.rs diff --git a/tools/aconfig/src/cache.rs b/tools/aconfig/src/cache.rs index 30810fa344..44ad3dd005 100644 --- a/tools/aconfig/src/cache.rs +++ b/tools/aconfig/src/cache.rs @@ -19,6 +19,7 @@ use serde::{Deserialize, Serialize}; use std::io::{Read, Write}; use crate::aconfig::{FlagDeclaration, FlagState, FlagValue, Permission}; +use crate::codegen; use crate::commands::Source; const DEFAULT_FLAG_STATE: FlagState = FlagState::Disabled; @@ -108,7 +109,7 @@ pub struct CacheBuilder { impl CacheBuilder { pub fn new(namespace: String) -> Result { - ensure!(!namespace.is_empty(), "empty namespace"); + ensure!(codegen::is_valid_identifier(&namespace), "bad namespace"); let cache = Cache { namespace, items: vec![] }; Ok(CacheBuilder { cache }) } @@ -118,7 +119,7 @@ impl CacheBuilder { source: Source, declaration: FlagDeclaration, ) -> Result<&mut CacheBuilder> { - ensure!(!declaration.name.is_empty(), "empty flag name"); + ensure!(codegen::is_valid_identifier(&declaration.name), "bad flag name"); ensure!(!declaration.description.is_empty(), "empty flag description"); ensure!( self.cache.items.iter().all(|item| item.name != declaration.name), @@ -146,8 +147,8 @@ impl CacheBuilder { source: Source, value: FlagValue, ) -> Result<&mut CacheBuilder> { - ensure!(!value.namespace.is_empty(), "empty flag namespace"); - ensure!(!value.name.is_empty(), "empty flag name"); + ensure!(codegen::is_valid_identifier(&value.namespace), "bad flag namespace"); + ensure!(codegen::is_valid_identifier(&value.name), "bad flag name"); ensure!( value.namespace == self.cache.namespace, "failed to set values for flag {}/{} from {}: expected namespace {}", @@ -270,14 +271,14 @@ mod tests { .add_flag_value( Source::Memory, FlagValue { - namespace: "some-other-namespace".to_string(), + namespace: "some_other_namespace".to_string(), name: "foo".to_string(), state: FlagState::Enabled, permission: Permission::ReadOnly, }, ) .unwrap_err(); - assert_eq!(&format!("{:?}", error), "failed to set values for flag some-other-namespace/foo from : expected namespace ns"); + assert_eq!(&format!("{:?}", error), "failed to set values for flag some_other_namespace/foo from : expected namespace ns"); let cache = builder.build(); let item = cache.iter().find(|&item| item.name == "foo").unwrap(); @@ -300,7 +301,7 @@ mod tests { FlagDeclaration { name: "".to_string(), description: "Description".to_string() }, ) .unwrap_err(); - assert_eq!(&format!("{:?}", error), "empty flag name"); + assert_eq!(&format!("{:?}", error), "bad flag name"); let error = builder .add_flag_declaration( @@ -332,7 +333,7 @@ mod tests { }, ) .unwrap_err(); - assert_eq!(&format!("{:?}", error), "empty flag namespace"); + assert_eq!(&format!("{:?}", error), "bad flag namespace"); let error = builder .add_flag_value( @@ -345,7 +346,7 @@ mod tests { }, ) .unwrap_err(); - assert_eq!(&format!("{:?}", error), "empty flag name"); + assert_eq!(&format!("{:?}", error), "bad flag name"); } #[test] diff --git a/tools/aconfig/src/codegen.rs b/tools/aconfig/src/codegen.rs new file mode 100644 index 0000000000..b60ec51c1d --- /dev/null +++ b/tools/aconfig/src/codegen.rs @@ -0,0 +1,43 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +pub fn is_valid_identifier(s: &str) -> bool { + // Identifiers must match [a-z][a-z0-9_]* + let mut chars = s.chars(); + let Some(first) = chars.next() else { + return false; + }; + if !first.is_ascii_lowercase() { + return false; + } + chars.all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_') +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_is_valid_identifier() { + assert!(is_valid_identifier("foo")); + assert!(is_valid_identifier("foo_bar_123")); + + assert!(!is_valid_identifier("")); + assert!(!is_valid_identifier("123_foo")); + assert!(!is_valid_identifier("foo-bar")); + assert!(!is_valid_identifier("foo-b\u{00e5}r")); + } +} diff --git a/tools/aconfig/src/codegen_java.rs b/tools/aconfig/src/codegen_java.rs index 733b1c5d1f..98288e7b2b 100644 --- a/tools/aconfig/src/codegen_java.rs +++ b/tools/aconfig/src/codegen_java.rs @@ -31,7 +31,7 @@ pub fn generate_java_code(cache: &Cache) -> Result { let mut template = TinyTemplate::new(); template.add_template("java_code_gen", include_str!("../templates/java.template"))?; let contents = template.render("java_code_gen", &context)?; - let mut path: PathBuf = namespace.split('.').collect(); + let mut path: PathBuf = ["aconfig", namespace].iter().collect(); // TODO: Allow customization of the java class name path.push("Flags.java"); Ok(OutputFile { contents: contents.into(), path }) @@ -76,7 +76,7 @@ mod tests { #[test] fn test_generate_java_code() { - let namespace = "com.example"; + let namespace = "example"; let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); builder .add_flag_declaration( @@ -106,7 +106,7 @@ mod tests { ) .unwrap(); let cache = builder.build(); - let expect_content = r#"package com.example; + let expect_content = r#"package aconfig.example; import android.provider.DeviceConfig; @@ -118,7 +118,7 @@ mod tests { public static boolean test2() { return DeviceConfig.getBoolean( - "com.example", + "example", "test2__test2", false ); @@ -127,7 +127,7 @@ mod tests { } "#; let file = generate_java_code(&cache).unwrap(); - assert_eq!("com/example/Flags.java", file.path.to_str().unwrap()); + assert_eq!("aconfig/example/Flags.java", file.path.to_str().unwrap()); assert_eq!( expect_content.replace(' ', ""), String::from_utf8(file.contents).unwrap().replace(' ', "") diff --git a/tools/aconfig/src/codegen_rust.rs b/tools/aconfig/src/codegen_rust.rs index d75e315f61..b3a6f53b73 100644 --- a/tools/aconfig/src/codegen_rust.rs +++ b/tools/aconfig/src/codegen_rust.rs @@ -23,10 +23,10 @@ use crate::cache::{Cache, Item}; use crate::commands::OutputFile; pub fn generate_rust_code(cache: &Cache) -> Result { - let namespace = cache.namespace().to_lowercase(); + let namespace = cache.namespace(); let parsed_flags: Vec = - cache.iter().map(|item| create_template_parsed_flag(&namespace, item)).collect(); - let context = TemplateContext { namespace, parsed_flags }; + cache.iter().map(|item| create_template_parsed_flag(namespace, item)).collect(); + let context = TemplateContext { namespace: namespace.to_string(), parsed_flags }; let mut template = TinyTemplate::new(); template.add_template("rust_code_gen", include_str!("../templates/rust.template"))?; let contents = template.render("rust_code_gen", &context)?; @@ -56,7 +56,7 @@ struct TemplateParsedFlag { fn create_template_parsed_flag(namespace: &str, item: &Item) -> TemplateParsedFlag { let template = TemplateParsedFlag { name: item.name.clone(), - fn_name: format!("{}_{}", namespace, item.name.replace('-', "_").to_lowercase()), + fn_name: format!("{}_{}", namespace, &item.name), is_read_only_enabled: item.permission == Permission::ReadOnly && item.state == FlagState::Enabled, is_read_only_disabled: item.permission == Permission::ReadOnly @@ -111,7 +111,7 @@ pub const fn r#test_disabled_ro() -> bool { #[inline(always)] pub fn r#test_disabled_rw() -> bool { - profcollect_libflags_rust::GetServerConfigurableFlag("test", "disabled-rw", "false") == "true" + profcollect_libflags_rust::GetServerConfigurableFlag("test", "disabled_rw", "false") == "true" } #[inline(always)] @@ -121,7 +121,7 @@ pub const fn r#test_enabled_ro() -> bool { #[inline(always)] pub fn r#test_enabled_rw() -> bool { - profcollect_libflags_rust::GetServerConfigurableFlag("test", "enabled-rw", "false") == "true" + profcollect_libflags_rust::GetServerConfigurableFlag("test", "enabled_rw", "false") == "true" } "#; assert_eq!(expected.trim(), String::from_utf8(generated.contents).unwrap().trim()); diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index b60909b377..1d2ec95fec 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -26,6 +26,7 @@ use std::path::{Path, PathBuf}; mod aconfig; mod cache; +mod codegen; mod codegen_cpp; mod codegen_java; mod codegen_rust; diff --git a/tools/aconfig/templates/java.template b/tools/aconfig/templates/java.template index 89da18bc43..30c7ad7579 100644 --- a/tools/aconfig/templates/java.template +++ b/tools/aconfig/templates/java.template @@ -1,4 +1,4 @@ -package {namespace}; +package aconfig.{namespace}; {{ if readwrite }} import android.provider.DeviceConfig; {{ endif }} @@ -10,7 +10,7 @@ public final class Flags \{ "{namespace}", "{item.feature_name}__{item.flag_name}", {item.default_value} - ); + ); {{ -else- }} return {item.default_value}; {{ -endif }} diff --git a/tools/aconfig/testdata/first.values b/tools/aconfig/testdata/first.values index e6017fe841..3c49111e37 100644 --- a/tools/aconfig/testdata/first.values +++ b/tools/aconfig/testdata/first.values @@ -1,18 +1,18 @@ flag_value { namespace: "test" - name: "disabled-ro" + name: "disabled_ro" state: DISABLED permission: READ_ONLY } flag_value { namespace: "test" - name: "enabled-ro" + name: "enabled_ro" state: DISABLED permission: READ_WRITE } flag_value { namespace: "test" - name: "enabled-rw" + name: "enabled_rw" state: ENABLED permission: READ_WRITE } diff --git a/tools/aconfig/testdata/second.values b/tools/aconfig/testdata/second.values index 44b6b3e445..3fe11ab159 100644 --- a/tools/aconfig/testdata/second.values +++ b/tools/aconfig/testdata/second.values @@ -1,6 +1,6 @@ flag_value { namespace: "test" - name: "enabled-ro" + name: "enabled_ro" state: ENABLED permission: READ_ONLY } diff --git a/tools/aconfig/testdata/test.aconfig b/tools/aconfig/testdata/test.aconfig index 16be425839..986a526ef3 100644 --- a/tools/aconfig/testdata/test.aconfig +++ b/tools/aconfig/testdata/test.aconfig @@ -4,14 +4,14 @@ namespace: "test" # - test.aconfig: DISABLED + READ_WRITE (default) # - first.values: DISABLED + READ_ONLY flag { - name: "disabled-ro" + name: "disabled_ro" description: "This flag is DISABLED + READ_ONLY" } # This flag's final value is calculated from: # - test.aconfig: DISABLED + READ_WRITE (default) flag { - name: "disabled-rw" + name: "disabled_rw" description: "This flag is DISABLED + READ_WRITE" } @@ -20,7 +20,7 @@ flag { # - first.values: DISABLED + READ_WRITE # - second.values: ENABLED + READ_ONLY flag { - name: "enabled-ro" + name: "enabled_ro" description: "This flag is ENABLED + READ_ONLY" } @@ -28,6 +28,6 @@ flag { # - test.aconfig: DISABLED + READ_WRITE (default) # - first.values: ENABLED + READ_WRITE flag { - name: "enabled-rw" + name: "enabled_rw" description: "This flag is ENABLED + READ_WRITE" }