Browse Source

Make bootclasspath_fragment hidden API package checks exhaustive

Previously, the bootclasspath_fragment's hidden_api.split_packages and
hidden_api.package_prefixes properties did not specify an exhaustive
set of packages that were provided by the fragment. They excluded
packages which were either not split or which could not be used as a
package prefix because it would match sub-packages provided by other
bootclasspath modules.

This change adds the hidden_api.single_packages list to specify those
additional packages and then uses that information to verify that any
bootclasspath_fragment that specifies at least one of split_packages,
single_packages or package_prefixes properties only contains classes
from a package that matches one of those properties. That will
prevent a module from accidentally including unexpected classes, such
as might happen when statically including a common utility library.

It also adds coverage specific versions of the properties as additional
packages are added to the art-bootclasspath-fragment when building
coverage builds.

Bug: 194063708
Test: atest signature_patterns_test
      m out/soong/hiddenapi/hiddenapi-flags.csv
      m EMMA_INSTRUMENT=true EMMA_INSTRUMENT_FRAMEWORK=true out/soong/hiddenapi/hiddenapi-flags.csv
      # Breaks without corresponding change to add android.system to
      # the art-bootclasspath-fragment.
      /usr/bin/pylint --rcfile $ANDROID_BUILD_TOP/tools/repohooks/tools/pylintrc scripts/hiddenapi/signature_patterns*.py
      pyformat -s 4 --force_quote_type single -i scripts/hiddenapi/signature_patterns*.py
Change-Id: Iddf6c59cd4dc8c36dde7943a9840ccef5794b320
Paul Duffin 2 years ago
parent
commit
846beb7f54

+ 26 - 4
java/bootclasspath_fragment.go

@@ -140,7 +140,7 @@ type bootclasspathFragmentProperties struct {
 	BootclasspathFragmentsDepsProperties
 }
 
-type SourceOnlyBootclasspathProperties struct {
+type HiddenApiPackageProperties struct {
 	Hidden_api struct {
 		// Contains prefixes of a package hierarchy that is provided solely by this
 		// bootclasspath_fragment.
@@ -149,6 +149,14 @@ type SourceOnlyBootclasspathProperties struct {
 		// hidden API flags. See split_packages property for more details.
 		Package_prefixes []string
 
+		// A list of individual packages that are provided solely by this
+		// bootclasspath_fragment but which cannot be listed in package_prefixes
+		// because there are sub-packages which are provided by other modules.
+		//
+		// This should only be used for legacy packages. New packages should be
+		// covered by a package prefix.
+		Single_packages []string
+
 		// The list of split packages provided by this bootclasspath_fragment.
 		//
 		// A split package is one that contains classes which are provided by multiple
@@ -208,6 +216,11 @@ type SourceOnlyBootclasspathProperties struct {
 	}
 }
 
+type SourceOnlyBootclasspathProperties struct {
+	HiddenApiPackageProperties
+	Coverage HiddenApiPackageProperties
+}
+
 type BootclasspathFragmentModule struct {
 	android.ModuleBase
 	android.ApexModuleBase
@@ -271,6 +284,12 @@ func bootclasspathFragmentFactory() android.Module {
 				ctx.PropertyErrorf("coverage", "error trying to append coverage specific properties: %s", err)
 				return
 			}
+
+			err = proptools.AppendProperties(&m.sourceOnlyProperties.HiddenApiPackageProperties, &m.sourceOnlyProperties.Coverage, nil)
+			if err != nil {
+				ctx.PropertyErrorf("coverage", "error trying to append hidden api coverage specific properties: %s", err)
+				return
+			}
 		}
 
 		// Initialize the contents property from the image_name.
@@ -731,7 +750,8 @@ func (b *BootclasspathFragmentModule) generateHiddenAPIBuildActions(ctx android.
 	// TODO(b/192868581): Remove once the source and prebuilts provide a signature patterns file of
 	//  their own.
 	if output.SignaturePatternsPath == nil {
-		output.SignaturePatternsPath = buildRuleSignaturePatternsFile(ctx, output.AllFlagsPath, []string{"*"}, nil)
+		output.SignaturePatternsPath = buildRuleSignaturePatternsFile(
+			ctx, output.AllFlagsPath, []string{"*"}, nil, nil)
 	}
 
 	// Initialize a HiddenAPIInfo structure.
@@ -806,11 +826,13 @@ func (b *BootclasspathFragmentModule) produceHiddenAPIOutput(ctx android.ModuleC
 	// signature patterns.
 	splitPackages := b.sourceOnlyProperties.Hidden_api.Split_packages
 	packagePrefixes := b.sourceOnlyProperties.Hidden_api.Package_prefixes
-	if splitPackages != nil || packagePrefixes != nil {
+	singlePackages := b.sourceOnlyProperties.Hidden_api.Single_packages
+	if splitPackages != nil || packagePrefixes != nil || singlePackages != nil {
 		if splitPackages == nil {
 			splitPackages = []string{"*"}
 		}
-		output.SignaturePatternsPath = buildRuleSignaturePatternsFile(ctx, output.AllFlagsPath, splitPackages, packagePrefixes)
+		output.SignaturePatternsPath = buildRuleSignaturePatternsFile(
+			ctx, output.AllFlagsPath, splitPackages, packagePrefixes, singlePackages)
 	}
 
 	return output

+ 4 - 1
java/hiddenapi_modular.go

@@ -943,7 +943,9 @@ func (s SignatureCsvSubsets) RelativeToTop() []string {
 
 // buildRuleSignaturePatternsFile creates a rule to generate a file containing the set of signature
 // patterns that will select a subset of the monolithic flags.
-func buildRuleSignaturePatternsFile(ctx android.ModuleContext, flagsPath android.Path, splitPackages []string, packagePrefixes []string) android.Path {
+func buildRuleSignaturePatternsFile(
+	ctx android.ModuleContext, flagsPath android.Path,
+	splitPackages []string, packagePrefixes []string, singlePackages []string) android.Path {
 	patternsFile := android.PathForModuleOut(ctx, "modular-hiddenapi", "signature-patterns.csv")
 	// Create a rule to validate the output from the following rule.
 	rule := android.NewRuleBuilder(pctx, ctx)
@@ -959,6 +961,7 @@ func buildRuleSignaturePatternsFile(ctx android.ModuleContext, flagsPath android
 		FlagWithInput("--flags ", flagsPath).
 		FlagForEachArg("--split-package ", quotedSplitPackages).
 		FlagForEachArg("--package-prefix ", packagePrefixes).
+		FlagForEachArg("--single-package ", singlePackages).
 		FlagWithOutput("--output ", patternsFile)
 	rule.Build("hiddenAPISignaturePatterns", "hidden API signature patterns")
 

+ 98 - 24
scripts/hiddenapi/signature_patterns.py

@@ -60,7 +60,22 @@ def matched_by_package_prefix_pattern(package_prefixes, prefix):
     return False
 
 
-def validate_package_prefixes(split_packages, package_prefixes):
+def validate_package_is_not_matched_by_package_prefix(package_type, pkg,
+                                                      package_prefixes):
+    package_prefix = matched_by_package_prefix_pattern(package_prefixes, pkg)
+    if package_prefix:
+        # A package prefix matches the package.
+        package_for_output = slash_package_to_dot_package(pkg)
+        package_prefix_for_output = slash_package_to_dot_package(package_prefix)
+        return [
+            f'{package_type} {package_for_output} is matched by '
+            f'package prefix {package_prefix_for_output}'
+        ]
+    return []
+
+
+def validate_package_prefixes(split_packages, single_packages,
+                              package_prefixes):
     # If there are no package prefixes then there is no possible conflict
     # between them and the split packages.
     if len(package_prefixes) == 0:
@@ -79,17 +94,16 @@ def validate_package_prefixes(split_packages, package_prefixes):
                 f'{package_prefixes_for_output}\n'
                 '    add split_packages:[] to fix')
         else:
-            package_prefix = matched_by_package_prefix_pattern(
-                package_prefixes, split_package)
-            if package_prefix:
-                # A package prefix matches a split package.
-                split_package_for_output = slash_package_to_dot_package(
-                    split_package)
-                package_prefix_for_output = slash_package_to_dot_package(
-                    package_prefix)
-                errors.append(
-                    f'split package {split_package_for_output} is matched by '
-                    f'package prefix {package_prefix_for_output}')
+            errs = validate_package_is_not_matched_by_package_prefix(
+                'split package', split_package, package_prefixes)
+            errors.extend(errs)
+
+    # Check to make sure that the single packages and package prefixes do not
+    # overlap.
+    for single_package in single_packages:
+        errs = validate_package_is_not_matched_by_package_prefix(
+            'single package', single_package, package_prefixes)
+        errors.extend(errs)
     return errors
 
 
@@ -102,21 +116,40 @@ def validate_split_packages(split_packages):
     return errors
 
 
+def validate_single_packages(split_packages, single_packages):
+    overlaps = []
+    for single_package in single_packages:
+        if single_package in split_packages:
+            overlaps.append(single_package)
+    if overlaps:
+        indented = ''.join([f'\n    {o}' for o in overlaps])
+        return [
+            f'single_packages and split_packages overlap, please ensure the '
+            f'following packages are only present in one:{indented}'
+        ]
+    return []
+
+
 def produce_patterns_from_file(file,
                                split_packages=None,
+                               single_packages=None,
                                package_prefixes=None):
     with open(file, 'r', encoding='utf8') as f:
-        return produce_patterns_from_stream(f, split_packages, package_prefixes)
+        return produce_patterns_from_stream(f, split_packages, single_packages,
+                                            package_prefixes)
 
 
 def produce_patterns_from_stream(stream,
                                  split_packages=None,
+                                 single_packages=None,
                                  package_prefixes=None):
     split_packages = set(split_packages or [])
+    single_packages = set(single_packages or [])
     package_prefixes = list(package_prefixes or [])
     # Read in all the signatures into a list and remove any unnecessary class
     # and member names.
     patterns = set()
+    unmatched_packages = set()
     for row in dict_reader(stream):
         signature = row['signature']
         text = signature.removeprefix('L')
@@ -138,11 +171,31 @@ def produce_patterns_from_stream(stream,
             # Remove inner class names.
             pieces = qualified_class_name.split('$', maxsplit=1)
             pattern = pieces[0]
-        else:
+            patterns.add(pattern)
+        elif pkg in single_packages:
             # Add a * to ensure that the pattern matches the classes in that
             # package.
             pattern = pkg + '/*'
-        patterns.add(pattern)
+            patterns.add(pattern)
+        else:
+            unmatched_packages.add(pkg)
+
+    # Remove any unmatched packages that would be matched by a package prefix
+    # pattern.
+    unmatched_packages = [
+        p for p in unmatched_packages
+        if not matched_by_package_prefix_pattern(package_prefixes, p)
+    ]
+    errors = []
+    if unmatched_packages:
+        unmatched_packages.sort()
+        indented = ''.join([
+            f'\n    {slash_package_to_dot_package(p)}'
+            for p in unmatched_packages
+        ])
+        errors.append('The following packages were unexpected, please add them '
+                      'to one of the hidden_api properties, split_packages, '
+                      f'single_packages or package_prefixes:{indented}')
 
     # Remove any patterns that would be matched by a package prefix pattern.
     patterns = [
@@ -155,7 +208,13 @@ def produce_patterns_from_stream(stream,
     patterns = patterns + [f'{p}/**' for p in package_prefixes]
     # Sort the patterns.
     patterns.sort()
-    return patterns
+    return patterns, errors
+
+
+def print_and_exit(errors):
+    for error in errors:
+        print(error)
+    sys.exit(1)
 
 
 def main(args):
@@ -175,26 +234,41 @@ def main(args):
         '--package-prefix',
         action='append',
         help='A package prefix unique to this set of flags')
+    args_parser.add_argument(
+        '--single-package',
+        action='append',
+        help='A single package unique to this set of flags')
     args_parser.add_argument('--output', help='Generated signature prefixes')
     args = args_parser.parse_args(args)
 
     split_packages = set(
         dot_packages_to_slash_packages(args.split_package or []))
     errors = validate_split_packages(split_packages)
+    if errors:
+        print_and_exit(errors)
 
-    package_prefixes = dot_packages_to_slash_packages(args.package_prefix or [])
+    single_packages = list(
+        dot_packages_to_slash_packages(args.single_package or []))
 
-    if not errors:
-        errors = validate_package_prefixes(split_packages, package_prefixes)
+    errors = validate_single_packages(split_packages, single_packages)
+    if errors:
+        print_and_exit(errors)
+
+    package_prefixes = dot_packages_to_slash_packages(args.package_prefix or [])
 
+    errors = validate_package_prefixes(split_packages, single_packages,
+                                       package_prefixes)
     if errors:
-        for error in errors:
-            print(error)
-        sys.exit(1)
+        print_and_exit(errors)
 
+    patterns = []
     # Read in all the patterns into a list.
-    patterns = produce_patterns_from_file(args.flags, split_packages,
-                                          package_prefixes)
+    patterns, errors = produce_patterns_from_file(args.flags, split_packages,
+                                                  single_packages,
+                                                  package_prefixes)
+
+    if errors:
+        print_and_exit(errors)
 
     # Write out all the patterns.
     with open(args.output, 'w', encoding='utf8') as outputFile:

+ 48 - 10
scripts/hiddenapi/signature_patterns_test.py

@@ -32,22 +32,37 @@ Ljava/lang/Object;->toString()Ljava/lang/String;,blocked
     @staticmethod
     def produce_patterns_from_string(csv_text,
                                      split_packages=None,
+                                     single_packages=None,
                                      package_prefixes=None):
         with io.StringIO(csv_text) as f:
             return signature_patterns.produce_patterns_from_stream(
-                f, split_packages, package_prefixes)
+                f, split_packages, single_packages, package_prefixes)
 
-    def test_generate_default(self):
-        patterns = self.produce_patterns_from_string(
+    def test_generate_unmatched(self):
+        _, errors = self.produce_patterns_from_string(
             TestGeneratedPatterns.csv_flags)
+        self.assertEqual([
+            'The following packages were unexpected, please add them to one of '
+            'the hidden_api properties, split_packages, single_packages or '
+            'package_prefixes:\n'
+            '    java.lang'
+        ], errors)
+
+    def test_generate_default(self):
+        patterns, errors = self.produce_patterns_from_string(
+            TestGeneratedPatterns.csv_flags, single_packages=['java/lang'])
+        self.assertEqual([], errors)
+
         expected = [
             'java/lang/*',
         ]
         self.assertEqual(expected, patterns)
 
     def test_generate_split_package(self):
-        patterns = self.produce_patterns_from_string(
+        patterns, errors = self.produce_patterns_from_string(
             TestGeneratedPatterns.csv_flags, split_packages={'java/lang'})
+        self.assertEqual([], errors)
+
         expected = [
             'java/lang/Character',
             'java/lang/Object',
@@ -56,8 +71,10 @@ Ljava/lang/Object;->toString()Ljava/lang/String;,blocked
         self.assertEqual(expected, patterns)
 
     def test_generate_split_package_wildcard(self):
-        patterns = self.produce_patterns_from_string(
+        patterns, errors = self.produce_patterns_from_string(
             TestGeneratedPatterns.csv_flags, split_packages={'*'})
+        self.assertEqual([], errors)
+
         expected = [
             'java/lang/Character',
             'java/lang/Object',
@@ -66,16 +83,20 @@ Ljava/lang/Object;->toString()Ljava/lang/String;,blocked
         self.assertEqual(expected, patterns)
 
     def test_generate_package_prefix(self):
-        patterns = self.produce_patterns_from_string(
+        patterns, errors = self.produce_patterns_from_string(
             TestGeneratedPatterns.csv_flags, package_prefixes={'java/lang'})
+        self.assertEqual([], errors)
+
         expected = [
             'java/lang/**',
         ]
         self.assertEqual(expected, patterns)
 
     def test_generate_package_prefix_top_package(self):
-        patterns = self.produce_patterns_from_string(
+        patterns, errors = self.produce_patterns_from_string(
             TestGeneratedPatterns.csv_flags, package_prefixes={'java'})
+        self.assertEqual([], errors)
+
         expected = [
             'java/**',
         ]
@@ -92,21 +113,38 @@ Ljava/lang/Object;->toString()Ljava/lang/String;,blocked
 
     def test_split_package_wildcard_conflicts_with_package_prefixes(self):
         errors = signature_patterns.validate_package_prefixes(
-            {'*'}, package_prefixes={'java'})
+            {'*'}, [], package_prefixes={'java'})
         expected = [
             "split package '*' conflicts with all package prefixes java\n"
             '    add split_packages:[] to fix',
         ]
         self.assertEqual(expected, errors)
 
-    def test_split_package_conflict(self):
+    def test_split_package_conflicts_with_package_prefixes(self):
         errors = signature_patterns.validate_package_prefixes(
-            {'java/split'}, package_prefixes={'java'})
+            {'java/split'}, [], package_prefixes={'java'})
         expected = [
             'split package java.split is matched by package prefix java',
         ]
         self.assertEqual(expected, errors)
 
+    def test_single_package_conflicts_with_package_prefixes(self):
+        errors = signature_patterns.validate_package_prefixes(
+            {}, ['java/single'], package_prefixes={'java'})
+        expected = [
+            'single package java.single is matched by package prefix java',
+        ]
+        self.assertEqual(expected, errors)
+
+    def test_single_package_conflicts_with_split_packages(self):
+        errors = signature_patterns.validate_single_packages({'java/pkg'},
+                                                             ['java/pkg'])
+        expected = [
+            'single_packages and split_packages overlap, please ensure the '
+            'following packages are only present in one:\n    java/pkg'
+        ]
+        self.assertEqual(expected, errors)
+
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)