From 7d584e936005d7ac6439457a155cc59609e22940 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Fri, 23 Oct 2020 18:26:03 +0100 Subject: [PATCH] Retry: Make ConfiguredJarList immutable By making the Append and RemoveList methods return a new list instead of modifying the existing list it makes the ConfiguredJarList usages easier to reason about and safer to use, especially considering that they are primarily used in global configuration. Added some tests for Append/RemoveList to ensure that they work and do not modify the original or result in newly created lists sharing storage with the original which would lead to corruption. Bug: 171756871 Bug: 171479578 Test: m nothing EMMA_INSTRUMENT=true EMMA_INSTRUMENT_FRAMEWORK=true m nothing Change-Id: I541c4686ecdd45c6a0c8b1c93fedf0fcd5952e2b --- android/config.go | 41 +++++++++++++++++++++++++---------- android/config_test.go | 46 ++++++++++++++++++++++++++++++++++++++++ java/dexpreopt_config.go | 7 +++--- 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/android/config.go b/android/config.go index 1f70fea7a..dbae7f78c 100644 --- a/android/config.go +++ b/android/config.go @@ -1361,14 +1361,31 @@ func (l *ConfiguredJarList) IndexOfJar(jar string) int { return IndexList(jar, l.jars) } +func copyAndAppend(list []string, item string) []string { + // Create the result list to be 1 longer than the input. + result := make([]string, len(list)+1) + + // Copy the whole input list into the result. + count := copy(result, list) + + // Insert the extra item at the end. + result[count] = item + + return result +} + // Append an (apex, jar) pair to the list. -func (l *ConfiguredJarList) Append(apex string, jar string) { - l.apexes = append(l.apexes, apex) - l.jars = append(l.jars, jar) +func (l *ConfiguredJarList) Append(apex string, jar string) ConfiguredJarList { + // Create a copy of the backing arrays before appending to avoid sharing backing + // arrays that are mutated across instances. + apexes := copyAndAppend(l.apexes, apex) + jars := copyAndAppend(l.jars, jar) + + return ConfiguredJarList{apexes, jars} } // Filter out sublist. -func (l *ConfiguredJarList) RemoveList(list ConfiguredJarList) { +func (l *ConfiguredJarList) RemoveList(list ConfiguredJarList) ConfiguredJarList { apexes := make([]string, 0, l.Len()) jars := make([]string, 0, l.Len()) @@ -1380,13 +1397,7 @@ func (l *ConfiguredJarList) RemoveList(list ConfiguredJarList) { } } - l.apexes = apexes - l.jars = jars -} - -// A copy of itself. -func (l *ConfiguredJarList) CopyOf() ConfiguredJarList { - return ConfiguredJarList{CopyOf(l.apexes), CopyOf(l.jars)} + return ConfiguredJarList{apexes, jars} } // A copy of the list of strings containing jar components. @@ -1469,6 +1480,14 @@ func (l *ConfiguredJarList) DevicePaths(cfg Config, ostype OsType) []string { return paths } +func (l *ConfiguredJarList) String() string { + var pairs []string + for i := 0; i < l.Len(); i++ { + pairs = append(pairs, l.apexes[i]+":"+l.jars[i]) + } + return strings.Join(pairs, ",") +} + func splitListOfPairsIntoPairOfLists(list []string) ([]string, []string, error) { // Now we need to populate this list by splitting each item in the slice of // pairs and appending them to the appropriate list of apexes or jars. diff --git a/android/config_test.go b/android/config_test.go index 274d59fb6..68f68a0a5 100644 --- a/android/config_test.go +++ b/android/config_test.go @@ -91,3 +91,49 @@ func TestMissingVendorConfig(t *testing.T) { t.Errorf("Expected false") } } + +func assertStringEquals(t *testing.T, expected, actual string) { + if actual != expected { + t.Errorf("expected %q found %q", expected, actual) + } +} + +func TestConfiguredJarList(t *testing.T) { + list1 := CreateTestConfiguredJarList([]string{"apex1:jarA"}) + + t.Run("create", func(t *testing.T) { + assertStringEquals(t, "apex1:jarA", list1.String()) + }) + + list2 := list1.Append("apex2", "jarB") + t.Run("append", func(t *testing.T) { + assertStringEquals(t, "apex1:jarA,apex2:jarB", list2.String()) + }) + + t.Run("append does not modify", func(t *testing.T) { + assertStringEquals(t, "apex1:jarA", list1.String()) + }) + + // Make sure that two lists created by appending to the same list do not share storage. + list3 := list1.Append("apex3", "jarC") + t.Run("append does not share", func(t *testing.T) { + assertStringEquals(t, "apex1:jarA,apex2:jarB", list2.String()) + assertStringEquals(t, "apex1:jarA,apex3:jarC", list3.String()) + }) + + list4 := list3.RemoveList(list1) + t.Run("remove", func(t *testing.T) { + assertStringEquals(t, "apex3:jarC", list4.String()) + }) + + t.Run("remove does not modify", func(t *testing.T) { + assertStringEquals(t, "apex1:jarA,apex3:jarC", list3.String()) + }) + + // Make sure that two lists created by removing from the same list do not share storage. + list5 := list3.RemoveList(CreateTestConfiguredJarList([]string{"apex3:jarC"})) + t.Run("remove", func(t *testing.T) { + assertStringEquals(t, "apex3:jarC", list4.String()) + assertStringEquals(t, "apex1:jarA", list5.String()) + }) +} diff --git a/java/dexpreopt_config.go b/java/dexpreopt_config.go index 0f8888a7d..c315124c9 100644 --- a/java/dexpreopt_config.go +++ b/java/dexpreopt_config.go @@ -81,13 +81,12 @@ func genBootImageConfigs(ctx android.PathContext) map[string]*bootImageConfig { targets := dexpreoptTargets(ctx) deviceDir := android.PathForOutput(ctx, ctx.Config().DeviceName()) - artModules := global.ArtApexJars.CopyOf() + artModules := global.ArtApexJars // With EMMA_INSTRUMENT_FRAMEWORK=true the Core libraries depend on jacoco. if ctx.Config().IsEnvTrue("EMMA_INSTRUMENT_FRAMEWORK") { - artModules.Append("com.android.art", "jacocoagent") + artModules = artModules.Append("com.android.art", "jacocoagent") } - frameworkModules := global.BootJars.CopyOf() - frameworkModules.RemoveList(artModules) + frameworkModules := global.BootJars.RemoveList(artModules) artSubdir := "apex/art_boot_images/javalib" frameworkSubdir := "system/framework"