arcvm: Only expand ro.* properties

This will prevent Chrome from adding 'import' lines in the future
which could cause a infinite import loop (by importing the combined
property file itself.) See crbug.com/1114279.

This makes our unit tests simpler too.

BUG=None
TEST=arc.Boot.vm, try

Change-Id: I66f58cf4fdce5e2e84de754755e9d90525eb666e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343505Reviewed-by: Yury Khmel <khmel@chromium.org>
Commit-Queue: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796583}

components/arc/session/arc_property_util.cc

@@ -52,10 +52,6 @@ constexpr char kDalvikVmIsaArm64[] = "ro.dalvik.vm.isa.arm64=x86_64";
 // Maximum length of an Android property value.
 constexpr int kAndroidMaxPropertyLength = 91;
 
-// The following 4 functions as well as the constants above are the _exact_ copy
-// of the ones in platform2/arc/setup/arc_setup_util.cc. After modifying code in
-// Chromium, make sure to reflect the changes to the platform2 side.
-
 bool FindProperty(const std::string& line_prefix_to_find,
                   std::string* out_prop,
                   const std::string& line) {
@@ -155,6 +151,12 @@ std::string ComputeOEMKey(brillo::CrosConfigInterface* config,
   return board;
 }
 
+bool IsComment(const std::string& line) {
+  return base::StartsWith(
+      base::TrimWhitespaceASCII(line, base::TrimPositions::TRIM_LEADING), "#",
+      base::CompareCase::SENSITIVE);
+}
+
 bool ExpandPropertyContents(const std::string& content,
                             brillo::CrosConfigInterface* config,
                             std::string* expanded_content,
@@ -165,6 +167,16 @@ bool ExpandPropertyContents(const std::string& content,
 
   std::string new_properties;
   for (std::string line : lines) {
+    // Chrome only expands ro. properties at runtime.
+    if (!base::StartsWith(line, "ro.", base::CompareCase::SENSITIVE)) {
+      if (!IsComment(line) && line.find('{') != std::string::npos) {
+        // The non-ro property has substitution(s).
+        LOG(ERROR) << "Found substitution(s) in a non-ro property: " << line;
+        return false;
+      }
+      continue;
+    }
+
     // First expand {property} substitutions in the string.  The insertions
     // may contain substitutions of their own, so we need to repeat until
     // nothing more is found.

components/arc/session/arc_property_util_unittest.cc

@@ -49,8 +49,10 @@ TEST_F(ArcPropertyUtilTest, TestPropertyExpansions) {
 
   std::string expanded;
   EXPECT_TRUE(ExpandPropertyContentsForTesting(
-      "line1\n{brand}\nline3\n{brand} {brand}", &config, &expanded));
-  EXPECT_EQ("line1\nalphabet\nline3\nalphabet alphabet\n", expanded);
+      "ro.a=line1\nro.b={brand}\nro.c=line3\nro.d={brand} {brand}", &config,
+      &expanded));
+  EXPECT_EQ("ro.a=line1\nro.b=alphabet\nro.c=line3\nro.d=alphabet alphabet\n",
+            expanded);
 }
 
 TEST_F(ArcPropertyUtilTest, TestPropertyExpansionsUnmatchedBrace) {
@@ -58,8 +60,8 @@ TEST_F(ArcPropertyUtilTest, TestPropertyExpansionsUnmatchedBrace) {
   config.SetString("/arc/build-properties", "brand", "alphabet");
 
   std::string expanded;
-  EXPECT_FALSE(ExpandPropertyContentsForTesting("line{1\nline}2\nline3",
-                                                &config, &expanded));
+  EXPECT_FALSE(ExpandPropertyContentsForTesting(
+      "ro.a=line{1\nro.b=line}2\nro.c=line3", &config, &expanded));
 }
 
 TEST_F(ArcPropertyUtilTest, TestPropertyExpansionsRecursive) {
@@ -68,8 +70,9 @@ TEST_F(ArcPropertyUtilTest, TestPropertyExpansionsRecursive) {
   config.SetString("/arc/build-properties", "model", "{brand} soup");
 
   std::string expanded;
-  EXPECT_TRUE(ExpandPropertyContentsForTesting("{model}", &config, &expanded));
-  EXPECT_EQ("alphabet soup\n", expanded);
+  EXPECT_TRUE(
+      ExpandPropertyContentsForTesting("ro.a={model}", &config, &expanded));
+  EXPECT_EQ("ro.a=alphabet soup\n", expanded);
 }
 
 TEST_F(ArcPropertyUtilTest, TestPropertyExpansionsMissingProperty) {
@@ -78,9 +81,10 @@ TEST_F(ArcPropertyUtilTest, TestPropertyExpansionsMissingProperty) {
 
   std::string expanded;
 
-  EXPECT_FALSE(ExpandPropertyContentsForTesting("{missing-property}", &config,
-                                                &expanded));
-  EXPECT_FALSE(ExpandPropertyContentsForTesting("{model}", &config, &expanded));
+  EXPECT_FALSE(ExpandPropertyContentsForTesting("ro.a={missing-property}",
+                                                &config, &expanded));
+  EXPECT_FALSE(
+      ExpandPropertyContentsForTesting("ro.a={model}", &config, &expanded));
 }
 
 // Verify that ro.product.board gets copied to ro.oem.key1 as well.
@@ -215,7 +219,6 @@ TEST_F(ArcPropertyUtilTest, ExpandPropertyFile_NoExpansion) {
   EXPECT_TRUE(ExpandPropertyFileForTesting(path, dest, config()));
   std::string content;
   EXPECT_TRUE(base::ReadFileToString(dest, &content));
-  // Note: ExpandPropertyFileForTesting() adds a trailing LF.
   EXPECT_EQ(std::string(kValidProp) + "\n", content);
 }
 
@@ -234,7 +237,6 @@ TEST_F(ArcPropertyUtilTest, ExpandPropertyFile_Expansion) {
   EXPECT_TRUE(ExpandPropertyFileForTesting(path, dest, config()));
   std::string content;
   EXPECT_TRUE(base::ReadFileToString(dest, &content));
-  // Note: ExpandPropertyFileForTesting() adds a trailing LF.
   EXPECT_EQ("ro.foo=v1\nro.baz=v2\n", content);
 }
 
@@ -253,7 +255,6 @@ TEST_F(ArcPropertyUtilTest, ExpandPropertyFile_NestedExpansion) {
   EXPECT_TRUE(ExpandPropertyFileForTesting(path, dest, config()));
   std::string content;
   EXPECT_TRUE(base::ReadFileToString(dest, &content));
-  // Note: ExpandPropertyFileForTesting() adds a trailing LF.
   EXPECT_EQ("ro.foo=v2\nro.baz=v2\n", content);
 }
 
@@ -326,22 +327,21 @@ TEST_F(ArcPropertyUtilTest, ExpandPropertyFiles) {
   EXPECT_TRUE(base::PathExists(dest_dir.Append("vendor_build.prop")));
 
   // Verify their content.
-  // Note: ExpandPropertyFileForTesting() adds a trailing LF.
   std::string content;
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("default.prop"), &content));
-  EXPECT_EQ(std::string(kDefaultProp) + "\n", content);
+  EXPECT_EQ(std::string(kDefaultProp), content);
   EXPECT_TRUE(base::ReadFileToString(dest_dir.Append("build.prop"), &content));
-  EXPECT_EQ(std::string(kBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kBuildProp), content);
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("vendor_build.prop"), &content));
-  EXPECT_EQ(std::string(kVendorBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kVendorBuildProp), content);
 
   // Expand it again, verify the previous result is cleared.
   EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_dir, false, false));
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("default.prop"), &content));
-  EXPECT_EQ(std::string(kDefaultProp) + "\n", content);
+  EXPECT_EQ(std::string(kDefaultProp), content);
 
   // If default.prop does not exist in the source path, it should still process
   // the other files, while also ensuring that default.prop is removed from the
@@ -351,10 +351,10 @@ TEST_F(ArcPropertyUtilTest, ExpandPropertyFiles) {
   EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_dir, false, false));
 
   EXPECT_TRUE(base::ReadFileToString(dest_dir.Append("build.prop"), &content));
-  EXPECT_EQ(std::string(kBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kBuildProp), content);
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("vendor_build.prop"), &content));
-  EXPECT_EQ(std::string(kVendorBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kVendorBuildProp), content);
 
   // Finally, test the case where source is valid but the dest is not.
   EXPECT_FALSE(ExpandPropertyFiles(source_dir, base::FilePath("/nonexistent"),
@@ -406,27 +406,25 @@ TEST_F(ArcPropertyUtilTest, ExpandPropertyFiles_SingleFile) {
   EXPECT_TRUE(base::PathExists(dest_prop_file));
 
   // Verify the content.
-  // Note: ExpandPropertyFileForTesting() adds a trailing LF.
   std::string content;
   EXPECT_TRUE(base::ReadFileToString(dest_prop_file, &content));
-  EXPECT_EQ(base::StringPrintf("%s\n%s\n%s\n", kDefaultProp, kBuildProp,
-                               kVendorBuildProp),
-            content);
+  EXPECT_EQ(
+      base::StringPrintf("%s%s%s", kDefaultProp, kBuildProp, kVendorBuildProp),
+      content);
 
   // Expand it again, verify the previous result is cleared.
   EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_prop_file, true, false));
   EXPECT_TRUE(base::ReadFileToString(dest_prop_file, &content));
-  EXPECT_EQ(base::StringPrintf("%s\n%s\n%s\n", kDefaultProp, kBuildProp,
-                               kVendorBuildProp),
-            content);
+  EXPECT_EQ(
+      base::StringPrintf("%s%s%s", kDefaultProp, kBuildProp, kVendorBuildProp),
+      content);
 
   // If default.prop does not exist in the source path, it should still process
   // the other files.
   base::DeleteFile(source_dir.Append("default.prop"));
   EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_prop_file, true, false));
   EXPECT_TRUE(base::ReadFileToString(dest_prop_file, &content));
-  EXPECT_EQ(base::StringPrintf("%s\n%s\n", kBuildProp, kVendorBuildProp),
-            content);
+  EXPECT_EQ(base::StringPrintf("%s%s", kBuildProp, kVendorBuildProp), content);
 
   // Finally, test the case where source is valid but the dest is not.
   EXPECT_FALSE(ExpandPropertyFiles(source_dir, base::FilePath("/nonexistent"),
@@ -463,29 +461,29 @@ TEST_F(ArcPropertyUtilTest, TestNativeBridge64Support) {
   EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_dir, false, false));
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("default.prop"), &content));
-  EXPECT_EQ(std::string(kDefaultProp) + "\n", content);
+  EXPECT_EQ(std::string(kDefaultProp), content);
   EXPECT_TRUE(base::ReadFileToString(dest_dir.Append("build.prop"), &content));
-  EXPECT_EQ(std::string(kBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kBuildProp), content);
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("vendor_build.prop"), &content));
-  EXPECT_EQ(std::string(kVendorBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kVendorBuildProp), content);
 
   // Expand with experiment on, verify properties are added / modified in
   // build.prop but not other files.
   EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_dir, false, true));
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("default.prop"), &content));
-  EXPECT_EQ(std::string(kDefaultProp) + "\n", content);
+  EXPECT_EQ(std::string(kDefaultProp), content);
   EXPECT_TRUE(base::ReadFileToString(dest_dir.Append("build.prop"), &content));
   constexpr const char kBuildPropModified[] =
       "ro.baz=boo\n"
       "ro.product.cpu.abilist=x86_64,x86,arm64-v8a,armeabi-v7a,armeabi\n"
-      "ro.product.cpu.abilist64=x86_64,arm64-v8a\n\n"
-      "ro.dalvik.vm.isa.arm64=x86_64";
-  EXPECT_EQ(std::string(kBuildPropModified) + "\n", content);
+      "ro.product.cpu.abilist64=x86_64,arm64-v8a\n"
+      "ro.dalvik.vm.isa.arm64=x86_64\n";
+  EXPECT_EQ(std::string(kBuildPropModified), content);
   EXPECT_TRUE(
       base::ReadFileToString(dest_dir.Append("vendor_build.prop"), &content));
-  EXPECT_EQ(std::string(kVendorBuildProp) + "\n", content);
+  EXPECT_EQ(std::string(kVendorBuildProp), content);
 
   // Expand to a single file with experiment on, verify properties are added /
   // modified as expected.
@@ -497,7 +495,7 @@ TEST_F(ArcPropertyUtilTest, TestNativeBridge64Support) {
 
   // Verify the contents.
   EXPECT_TRUE(base::ReadFileToString(dest_prop_file, &content));
-  EXPECT_EQ(base::StringPrintf("%s\n%s\n%s\n", kDefaultProp, kBuildPropModified,
+  EXPECT_EQ(base::StringPrintf("%s%s%s", kDefaultProp, kBuildPropModified,
                                kVendorBuildProp),
             content);
 
@@ -518,5 +516,35 @@ TEST_F(ArcPropertyUtilTest, TestNativeBridge64Support) {
   EXPECT_FALSE(ExpandPropertyFiles(source_dir, dest_dir, false, true));
 }
 
+// Verify that comments and non ro. properties are not written.
+TEST_F(ArcPropertyUtilTest, ExpandPropertyFiles_SingleFile_NonRo) {
+  base::FilePath source_dir;
+  ASSERT_TRUE(base::CreateTemporaryDirInDir(GetTempDir(), "test", &source_dir));
+  base::FilePath dest_dir;
+  ASSERT_TRUE(base::CreateTemporaryDirInDir(GetTempDir(), "test", &dest_dir));
+
+  const base::FilePath default_prop = source_dir.Append("default.prop");
+  constexpr const char kDefaultProp[] = "###\ndalvik.foo=bar\nro.foo=bar\n";
+  base::WriteFile(default_prop, kDefaultProp, strlen(kDefaultProp));
+
+  const base::FilePath build_prop = source_dir.Append("build.prop");
+  constexpr const char kBuildProp[] = "###\ndalvik.baz=boo\nro.baz=boo\n";
+  base::WriteFile(build_prop, kBuildProp, strlen(kBuildProp));
+
+  const base::FilePath vendor_build_prop =
+      source_dir.Append("vendor_build.prop");
+  constexpr const char kVendorBuildProp[] = "###\ndalvik.a=b\nro.a=b\n";
+  base::WriteFile(vendor_build_prop, kVendorBuildProp,
+                  strlen(kVendorBuildProp));
+
+  const base::FilePath dest_prop_file = dest_dir.Append("combined.prop");
+  EXPECT_TRUE(ExpandPropertyFiles(source_dir, dest_prop_file, true, false));
+
+  // Verify the content.
+  std::string content;
+  EXPECT_TRUE(base::ReadFileToString(dest_prop_file, &content));
+  EXPECT_EQ("ro.foo=bar\nro.baz=boo\nro.a=b\n", content);
+}
+
 }  // namespace
 }  // namespace arc