-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fix method visibility in public-callers-finder #137200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
fccf83c
246db1a
56773b8
de7c2b8
30415c6
e3678ba
d197161
d3b4346
53b380c
78dcd4d
18f4455
1d55221
b4b3481
e68465f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,222 @@ | ||||||||||||
| /* | ||||||||||||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||||||||||||
| * or more contributor license agreements. Licensed under the "Elastic License | ||||||||||||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||||||||||||
| * Public License v 1"; you may not use this file except in compliance with, at | ||||||||||||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||||||||||||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||||||||||||
| */ | ||||||||||||
|
|
||||||||||||
| package org.elasticsearch.entitlement.tools; | ||||||||||||
|
|
||||||||||||
| import org.elasticsearch.core.Tuple; | ||||||||||||
| import org.objectweb.asm.ClassReader; | ||||||||||||
| import org.objectweb.asm.ClassVisitor; | ||||||||||||
| import org.objectweb.asm.MethodVisitor; | ||||||||||||
| import org.objectweb.asm.Opcodes; | ||||||||||||
|
|
||||||||||||
| import java.io.IOException; | ||||||||||||
| import java.lang.constant.ClassDesc; | ||||||||||||
| import java.util.Collections; | ||||||||||||
| import java.util.Comparator; | ||||||||||||
| import java.util.Map; | ||||||||||||
| import java.util.Set; | ||||||||||||
| import java.util.TreeMap; | ||||||||||||
| import java.util.TreeSet; | ||||||||||||
| import java.util.function.Predicate; | ||||||||||||
| import java.util.stream.Stream; | ||||||||||||
|
|
||||||||||||
| import static java.util.Collections.emptySet; | ||||||||||||
|
|
||||||||||||
| public class AccessibleJdkMethods { | ||||||||||||
|
|
||||||||||||
| private static final Set<AccessibleMethod.Descriptor> EXCLUDES = Set.of( | ||||||||||||
| new AccessibleMethod.Descriptor("toString", "()Ljava/lang/String;", true, false), | ||||||||||||
| new AccessibleMethod.Descriptor("hashCode", "()I", true, false), | ||||||||||||
| new AccessibleMethod.Descriptor("equals", "(Ljava/lang/Object;)Z", true, false), | ||||||||||||
| new AccessibleMethod.Descriptor("close", "()V", true, false) | ||||||||||||
| ); | ||||||||||||
|
|
||||||||||||
| public record AccessibleMethod(Descriptor descriptor, boolean isFinal, boolean isDeprecated) { | ||||||||||||
| public record Descriptor(String method, String descriptor, boolean isPublic, boolean isStatic) { | ||||||||||||
| public static final Comparator<Descriptor> COMPARATOR = Comparator.comparing(Descriptor::method) | ||||||||||||
| .thenComparing(Descriptor::descriptor) | ||||||||||||
| .thenComparing(Descriptor::isStatic); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| public static final Comparator<AccessibleMethod> COMPARATOR = Comparator.comparing( | ||||||||||||
| AccessibleMethod::descriptor, | ||||||||||||
| Descriptor.COMPARATOR | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| public record ModuleClass(String module, String clazz) { | ||||||||||||
| public static final Comparator<ModuleClass> COMPARATOR = Comparator.comparing(ModuleClass::module) | ||||||||||||
| .thenComparing(ModuleClass::clazz); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| public static Stream<Tuple<ModuleClass, AccessibleMethod>> loadAccessibleMethods(Predicate<String> modulePredicate) throws IOException { | ||||||||||||
| // 1st: map class names to module names (including later excluded modules) for lookup in 2nd step | ||||||||||||
| final Map<String, String> moduleNameByClass = Utils.loadClassToModuleMapping(); | ||||||||||||
| final Map<String, Set<String>> exportsByModule = Utils.loadExportsByModule(); | ||||||||||||
| final AccessibleMethodsVisitor visitor = new AccessibleMethodsVisitor(modulePredicate, moduleNameByClass, exportsByModule); | ||||||||||||
| // 2nd: calculate accessible implementations of classes in included modules | ||||||||||||
| Utils.walkJdkModules(modulePredicate, exportsByModule, (moduleName, moduleClasses, moduleExports) -> { | ||||||||||||
| for (var classFile : moduleClasses) { | ||||||||||||
| // visit class once (skips if class was already visited earlier due to a dependency on it) | ||||||||||||
| visitor.visitOnce(new ModuleClass(moduleName, Utils.internalClassName(classFile, moduleName))); | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| return visitor.getAccessibleMethods().entrySet().stream().flatMap(e -> e.getValue().stream().map(m -> Tuple.tuple(e.getKey(), m))); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private static class AccessibleMethodsVisitor extends ClassVisitor { | ||||||||||||
| private final Map<ModuleClass, Set<AccessibleMethod>> inheritableAccessByClass = new TreeMap<>(ModuleClass.COMPARATOR); | ||||||||||||
| private final Map<ModuleClass, Set<AccessibleMethod>> accessibleImplementationsByClass = new TreeMap<>(ModuleClass.COMPARATOR); | ||||||||||||
|
|
||||||||||||
| private final Predicate<String> modulePredicate; | ||||||||||||
| private final Map<String, String> moduleNameByClass; | ||||||||||||
| private final Map<String, Set<String>> exportsByModule; | ||||||||||||
|
|
||||||||||||
| private Set<AccessibleMethod> accessibleImplementations; | ||||||||||||
| private Set<AccessibleMethod> inheritableAccess; | ||||||||||||
|
|
||||||||||||
| private ModuleClass moduleClass; | ||||||||||||
| private boolean isPublicClass; | ||||||||||||
| private boolean isFinalClass; | ||||||||||||
| private boolean isDeprecatedClass; | ||||||||||||
| private boolean isExported; | ||||||||||||
|
|
||||||||||||
| AccessibleMethodsVisitor( | ||||||||||||
| Predicate<String> modulePredicate, | ||||||||||||
| Map<String, String> moduleNameByClass, | ||||||||||||
| Map<String, Set<String>> exportsByModule | ||||||||||||
| ) { | ||||||||||||
| super(Opcodes.ASM9); | ||||||||||||
| this.modulePredicate = modulePredicate; | ||||||||||||
| this.moduleNameByClass = moduleNameByClass; | ||||||||||||
| this.exportsByModule = exportsByModule; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private static Set<AccessibleMethod> newSortedSet() { | ||||||||||||
| return new TreeSet<>(AccessibleMethod.COMPARATOR); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| Map<ModuleClass, Set<AccessibleMethod>> getAccessibleMethods() { | ||||||||||||
| return Collections.unmodifiableMap(accessibleImplementationsByClass); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| void visitOnce(ModuleClass moduleClass) { | ||||||||||||
| if (accessibleImplementationsByClass.containsKey(moduleClass)) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
| if (moduleClass.clazz.startsWith("com/sun/") && moduleClass.clazz.contains("/internal/")) { | ||||||||||||
| // skip com.sun.*.internal classes as they are not part of the supported JDK API | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do wonder though: should we instrument those as always denied? If they are publicly accessible?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a tricky one, some of these implement public accessible interfaces. Theoretically, if leaked / returned somewhere we could invoke such code in user land. And obviously, without this filter, these would be reported.
But I agree, we probably want to verify that assumption. We could deny any access to such classes from "user code" or at least log warnings.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously this was done at the very end prior to writing the result, see Lines 112 to 116 in 80d91ce
In fact, that's the same as not visiting such classes at all. If you prefer to not filter these I'll remove, certainly not as harmful as the exclusions in #137193.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That what I was thinking too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created a follow up task for this |
||||||||||||
| // even if methods override some publicly visible API | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
| try { | ||||||||||||
| ClassReader cr = new ClassReader(moduleClass.clazz); | ||||||||||||
| cr.accept(this, 0); | ||||||||||||
| } catch (IOException e) { | ||||||||||||
| throw new RuntimeException(e); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Override | ||||||||||||
| public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { | ||||||||||||
| final Set<AccessibleMethod> currentInheritedAccess = newSortedSet(); | ||||||||||||
| if (superName != null) { | ||||||||||||
| var superModuleClass = getModuleClass(superName); | ||||||||||||
| visitOnce(superModuleClass); | ||||||||||||
| currentInheritedAccess.addAll(inheritableAccessByClass.getOrDefault(superModuleClass, emptySet())); | ||||||||||||
| } | ||||||||||||
| if (interfaces != null && interfaces.length > 0) { | ||||||||||||
| for (var interfaceName : interfaces) { | ||||||||||||
| var interfaceModuleClass = getModuleClass(interfaceName); | ||||||||||||
| visitOnce(interfaceModuleClass); | ||||||||||||
| currentInheritedAccess.addAll(inheritableAccessByClass.getOrDefault(interfaceModuleClass, emptySet())); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| // only initialize local state AFTER visiting all dependencies above! | ||||||||||||
| super.visit(version, access, name, signature, superName, interfaces); | ||||||||||||
| this.moduleClass = getModuleClass(name); | ||||||||||||
| this.isExported = getModuleExports(moduleClass.module()).contains(getPackageName(name)); | ||||||||||||
| this.isPublicClass = (access & Opcodes.ACC_PUBLIC) != 0; | ||||||||||||
| this.isFinalClass = (access & Opcodes.ACC_FINAL) != 0; | ||||||||||||
| this.isDeprecatedClass = (access & Opcodes.ACC_DEPRECATED) != 0; | ||||||||||||
| this.inheritableAccess = currentInheritedAccess; | ||||||||||||
| this.accessibleImplementations = newSortedSet(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private ModuleClass getModuleClass(String name) { | ||||||||||||
|
||||||||||||
| String module = moduleNameByClass.get(name); | ||||||||||||
| if (module == null) { | ||||||||||||
| throw new IllegalStateException("Unknown module for class: " + name); | ||||||||||||
| } | ||||||||||||
| return new ModuleClass(module, name); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private Set<String> getModuleExports(String module) { | ||||||||||||
| Set<String> exports = exportsByModule.get(module); | ||||||||||||
| if (exports == null) { | ||||||||||||
| throw new IllegalStateException("Unknown exports for module: " + module); | ||||||||||||
| } | ||||||||||||
| return exports; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Override | ||||||||||||
| public void visitEnd() { | ||||||||||||
| super.visitEnd(); | ||||||||||||
| if (accessibleImplementationsByClass.put(moduleClass, unmodifiableSet(accessibleImplementations)) != null | ||||||||||||
| || inheritableAccessByClass.put(moduleClass, unmodifiableSet(inheritableAccess)) != null) { | ||||||||||||
| throw new IllegalStateException("Class " + moduleClass.clazz() + " was already visited!"); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private static Set<AccessibleMethod> unmodifiableSet(Set<AccessibleMethod> set) { | ||||||||||||
| return set.isEmpty() ? emptySet() : Collections.unmodifiableSet(set); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private static String getPackageName(String className) { | ||||||||||||
| return ClassDesc.ofInternalName(className).packageName(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Override | ||||||||||||
| public final MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) { | ||||||||||||
| MethodVisitor mv = super.visitMethod(access, name, descriptor, signature, exceptions); | ||||||||||||
| boolean isPublic = (access & Opcodes.ACC_PUBLIC) != 0; | ||||||||||||
| boolean isProtected = (access & Opcodes.ACC_PROTECTED) != 0; | ||||||||||||
| boolean isFinal = (access & Opcodes.ACC_FINAL) != 0; | ||||||||||||
| boolean isStatic = (access & Opcodes.ACC_STATIC) != 0; | ||||||||||||
| boolean isDeprecated = (access & Opcodes.ACC_DEPRECATED) != 0; | ||||||||||||
| if ((isPublic || isProtected) == false) { | ||||||||||||
| return mv; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| var methodDescriptor = new AccessibleMethod.Descriptor(name, descriptor, isPublic, isStatic); | ||||||||||||
| var method = new AccessibleMethod(methodDescriptor, isFinal, isDeprecatedClass || isDeprecated); | ||||||||||||
| if (isPublicClass && isExported && EXCLUDES.contains(methodDescriptor) == false) { | ||||||||||||
| // class is public and exported, to be accessible outside the JDK the method must be either: | ||||||||||||
| // - public or | ||||||||||||
| // - protected if not a final class | ||||||||||||
| if (isPublic || isFinalClass == false) { | ||||||||||||
| if (modulePredicate.test(moduleClass.module)) { | ||||||||||||
| accessibleImplementations.add(method); | ||||||||||||
| } | ||||||||||||
| // if public and not static, the method can be accessible on non-public and non-exported subclasses, | ||||||||||||
| // but skip constructors | ||||||||||||
| if (isPublic && isStatic == false && name.equals("<init>") == false) { | ||||||||||||
| inheritableAccess.add(method); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } else if (inheritableAccess.contains(method)) { | ||||||||||||
| if (modulePredicate.test(moduleClass.module)) { | ||||||||||||
| accessibleImplementations.add(method); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| return mv; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,12 +24,7 @@ public static String toString(EnumSet<ExternalAccess> externalAccesses) { | |
| return externalAccesses.stream().map(Enum::toString).collect(Collectors.joining(DELIMITER)); | ||
| } | ||
|
|
||
| public static EnumSet<ExternalAccess> fromPermissions( | ||
| boolean packageExported, | ||
| boolean publicClass, | ||
| boolean publicMethod, | ||
| boolean protectedMethod | ||
| ) { | ||
| public static EnumSet<ExternalAccess> fromPermissions(boolean publicAccessible, boolean publicMethod, boolean protectedMethod) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do wonder if reducing to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was actually thinking the same, but stopped myself from making yet another change. But I think you're right, I'll have another look to see if this can be done with some decent effort.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll do as a follow up, this will impact a lot of places |
||
| if (publicMethod && protectedMethod) { | ||
| throw new IllegalArgumentException(); | ||
| } | ||
|
|
@@ -41,7 +36,7 @@ public static EnumSet<ExternalAccess> fromPermissions( | |
| externalAccesses.add(ExternalAccess.PROTECTED_METHOD); | ||
| } | ||
|
|
||
| if (packageExported && publicClass) { | ||
| if (publicAccessible) { | ||
| externalAccesses.add(ExternalAccess.PUBLIC_CLASS); | ||
| } | ||
| return externalAccesses; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| import java.nio.file.FileSystems; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -48,7 +49,11 @@ public class Utils { | |
| && m.contains(".internal.") == false | ||
| && m.contains(".incubator.") == false; | ||
|
|
||
| public static Map<String, Set<String>> findModuleExports() throws IOException { | ||
| public static final Predicate<String> modulePredicate(boolean includeIncubator) { | ||
| return includeIncubator == false ? DEFAULT_MODULE_PREDICATE : DEFAULT_MODULE_PREDICATE.or(m -> m.contains(".incubator.")); | ||
| } | ||
|
|
||
| public static Map<String, Set<String>> loadExportsByModule() throws IOException { | ||
| var modulesExports = new HashMap<String, Set<String>>(); | ||
| try (var stream = Files.walk(JRT_FS.getPath("modules"))) { | ||
| stream.filter(p -> p.getFileName().toString().equals("module-info.class")).forEach(x -> { | ||
|
|
@@ -67,15 +72,28 @@ public static Map<String, Set<String>> findModuleExports() throws IOException { | |
| } | ||
| }); | ||
| } | ||
| return modulesExports; | ||
| return Collections.unmodifiableMap(modulesExports); | ||
| } | ||
|
|
||
| public static Map<String, String> loadClassToModuleMapping() throws IOException { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved here from JdkApiExtractor |
||
| Map<String, String> moduleNameByClass = new HashMap<>(); | ||
| Utils.walkJdkModules(m -> true, Collections.emptyMap(), (moduleName, moduleClasses, moduleExports) -> { | ||
| for (var classFile : moduleClasses) { | ||
| String prev = moduleNameByClass.put(internalClassName(classFile, moduleName), moduleName); | ||
| if (prev != null) { | ||
| throw new IllegalStateException("Class " + classFile + " is in both modules " + prev + " and " + moduleName); | ||
| } | ||
| } | ||
| }); | ||
| return Collections.unmodifiableMap(moduleNameByClass); | ||
| } | ||
|
|
||
| public interface JdkModuleConsumer { | ||
| void accept(String moduleName, List<Path> moduleClasses, Set<String> moduleExports); | ||
| } | ||
|
|
||
| public static void walkJdkModules(JdkModuleConsumer c) throws IOException { | ||
| walkJdkModules(DEFAULT_MODULE_PREDICATE, Utils.findModuleExports(), c); | ||
| walkJdkModules(DEFAULT_MODULE_PREDICATE, Utils.loadExportsByModule(), c); | ||
| } | ||
|
|
||
| public static void walkJdkModules(Predicate<String> modulePredicate, Map<String, Set<String>> exportsByModule, JdkModuleConsumer c) | ||
|
|
@@ -88,10 +106,16 @@ public static void walkJdkModules(Predicate<String> modulePredicate, Map<String, | |
| for (var kv : modules.entrySet()) { | ||
| var moduleName = kv.getKey(); | ||
| if (modulePredicate.test(moduleName)) { | ||
| var thisModuleExports = exportsByModule.get(moduleName); | ||
| var thisModuleExports = exportsByModule.getOrDefault(moduleName, Collections.emptySet()); | ||
| c.accept(moduleName, kv.getValue(), thisModuleExports); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static String internalClassName(Path clazz, String moduleName) { | ||
| Path modulePath = clazz.getFileSystem().getPath("modules", moduleName); | ||
| String relativePath = modulePath.relativize(clazz).toString(); | ||
| return relativePath.substring(0, relativePath.length() - ".class".length()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moved here from JdkApiExtractor to make it accessible as common tool