Merge "aconfig: restrict valid namespace and flag names" am: cfc5f5e9fd

Original change: https://android-review.googlesource.com/c/platform/build/+/2607087

Change-Id: Ifb0c77ff4e3c51f8bd0dce787abb022946518c79
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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<CacheBuilder> {
-        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 <memory>: expected namespace ns");
+        assert_eq!(&format!("{:?}", error), "failed to set values for flag some_other_namespace/foo from <memory>: 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]

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"));
+    }
+}

tools/aconfig/src/codegen_java.rs

@@ -31,7 +31,7 @@ pub fn generate_java_code(cache: &Cache) -> Result<OutputFile> {
     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(' ', "")

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<OutputFile> {
-    let namespace = cache.namespace().to_lowercase();
+    let namespace = cache.namespace();
     let parsed_flags: Vec<TemplateParsedFlag> =
-        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());

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;

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

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
 }

tools/aconfig/testdata/second.values

@@ -1,6 +1,6 @@
 flag_value {
     namespace: "test"
-    name: "enabled-ro"
+    name: "enabled_ro"
     state: ENABLED
     permission: READ_ONLY
 }

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"
 }