Fix method visibility in public-callers-finder #137200

mosche · 2025-10-27T15:11:18Z

Public-callers-finder did not correctly report if methods are externally visible due to the following issues:

checking if interfaces are exported (see findAccessibility) must be done using the exports of the interface’s module, not the exports of the current class
super classes must be treated the same way as interfaces, currently these are ignored
currently all public methods of a (potentially private, non-exported) class implementing a public, exported interface are considered accessible / visible regardless if part of the interface or not

This fixes visibility to be consistent with the JdkApiExtractor tool by implementing both using the same common logic in AccessibleJdkMethods.loadAccessibleMethods.

Note: this currently includes #137193, which will be merged independently.

Relates to ES-13117

…llers finder

… public callers. Relates to ES-13117

Relates to ES-13117

elasticsearchmachine · 2025-10-27T15:12:27Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

mosche · 2025-10-28T07:51:35Z

...ent/tools/common/src/main/java/org/elasticsearch/entitlement/tools/AccessibleJdkMethods.java

+
+import static java.util.Collections.emptySet;
+
+public class AccessibleJdkMethods {


This is moved here from JdkApiExtractor to make it accessible as common tool

mosche · 2025-10-28T07:52:38Z

libs/entitlement/tools/common/src/main/java/org/elasticsearch/entitlement/tools/Utils.java

+        return Collections.unmodifiableMap(modulesExports);
+    }
+
+    public static Map<String, String> loadClassToModuleMapping() throws IOException {


Moved here from JdkApiExtractor

mosche · 2025-10-28T07:56:29Z

...ain/java/org/elasticsearch/entitlement/tools/publiccallersfinder/FindUsagesClassVisitor.java

-    protected FindUsagesClassVisitor(Set<String> moduleExports, MethodDescriptor methodToFind, CallerConsumer callers) {
+    protected FindUsagesClassVisitor(
+        MethodDescriptor methodToFind,
+        Predicate<MethodDescriptor> isPublicAccessible,


Rather than calculating visibility (and potentially having to follow interfaces / super classes), use a predicate based on a precalculated set of accessible method descriptors leveraging the new AccessibleJdkMethods

ldematte

Looks good!
I know this is "just a tool", but give its complexity I do wonder if some parts deserve "unit" tests. I'm thinking in particular at AccessibleJdkMethods and some of the logic in Main, e.g. findTransitiveUsages.
As a follow-up. Wdyt?

ldematte · 2025-10-28T08:03:32Z

...ent/tools/common/src/main/java/org/elasticsearch/entitlement/tools/AccessibleJdkMethods.java

+                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


I do wonder though: should we instrument those as always denied? If they are publicly accessible?
(surely not part of this PR! But it just occurred to me now)

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.
In the end this is purely based on the assumption / hope that no JDK API leaks such instances to keep the set of reported methods more manageable and avoid general noise on internal stuff when diffing between different JDKs.

I do wonder though: should we instrument those as always denied?

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.

Previously this was done at the very end prior to writing the result, see

elasticsearch/libs/entitlement/tools/jdk-api-extractor/src/main/java/org/elasticsearch/entitlement/tools/jdkapi/JdkApiExtractor.java

Lines 112 to 116 in 80d91ce

if (entry.getKey().clazz.startsWith("com/sun/") && entry.getKey().clazz.contains("/internal/")) {

// skip com.sun.*.internal classes as they are not part of the supported JDK API

// even if methods override some publicly visible API

return false;

}

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.

We could deny any access to such classes from "user code" or at least log warnings.

That what I was thinking too.
RE: this PR, I'm fine either way (keeping it as-is or remove the excludes)

I created a follow up task for this

ldematte · 2025-10-28T08:06:44Z

...ent/tools/common/src/main/java/org/elasticsearch/entitlement/tools/AccessibleJdkMethods.java

+            this.accessibleImplementations = newSortedSet();
+        }
+
+        private ModuleClass getModuleClass(String name) {


Nit: to make the code more readable, getModuleClassFromName?

ldematte · 2025-10-28T08:12:13Z

...titlement/tools/common/src/main/java/org/elasticsearch/entitlement/tools/ExternalAccess.java

-        boolean publicMethod,
-        boolean protectedMethod
-    ) {
+    public static EnumSet<ExternalAccess> fromPermissions(boolean publicAccessible, boolean publicMethod, boolean protectedMethod) {


I do wonder if reducing to ExternalAccess is still a good choice? Or if we should just dump/propagate these 3 booleans to the CSV?

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.

👍
Can definitely be a follow-up though

i'll do as a follow up, this will impact a lot of places

ldematte

LGTM!

Public-callers-finder did not correctly report if methods are externally visible due to the following issues: - checking if interfaces are exported (see findAccessibility) must be done using the exports of the interface’s module, not the exports of the current class - super classes must be treated the same way as interfaces, currently these are ignored - currently all public methods of a (potentially private, non-exported) class implementing a public, exported interface are considered accessible / visible regardless if part of the interface or not This fixes visibility to be consistent with the JdkApiExtractor tool by implementing both using the same common logic in `AccessibleJdkMethods.loadAccessibleMethods`. Note: this currently includes elastic#137193, which will be merged independently. Relates to ES-13117

mosche added 6 commits October 21, 2025 10:44

Made public jdk api extractor a common tool for easy use in public ca…

fccf83c

…llers finder

Merge branch 'main' into entitlements/fix-public-callers-visibility

246db1a

fix readme

56773b8

Exclude some internal, low-level classes from transitive searches for…

de7c2b8

… public callers. Relates to ES-13117

Use AccessibleJdkMethods to consistently calculate external visibility.

30415c6

Relates to ES-13117

spotless

e3678ba

mosche requested a review from a team as a code owner October 27, 2025 15:11

elasticsearchmachine added v9.3.0 needs:triage Requires assignment of a team area label labels Oct 27, 2025

mosche requested a review from ldematte October 27, 2025 15:11

mosche added :Core/Infra/Entitlements Entitlements infrastructure >refactoring and removed needs:triage Requires assignment of a team area label labels Oct 27, 2025

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Oct 27, 2025

Merge branch 'main' into entitlements/fix-public-callers-visibility

d197161

mosche commented Oct 28, 2025

View reviewed changes

ldematte reviewed Oct 28, 2025

View reviewed changes

mosche added 2 commits October 28, 2025 15:01

rename

d3b4346

Merge branch 'main' into entitlements/fix-public-callers-visibility

53b380c

ldematte approved these changes Oct 28, 2025

View reviewed changes

mosche added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 28, 2025

mosche added 5 commits October 29, 2025 12:17

fix licence check

78dcd4d

Merge branch 'main' into entitlements/fix-public-callers-visibility

18f4455

fix license check

1d55221

Merge branch 'main' into entitlements/fix-public-callers-visibility

b4b3481

fix thirdPartyAudit

e68465f

elasticsearchmachine merged commit 24a0c72 into elastic:main Oct 29, 2025
34 checks passed

mosche deleted the entitlements/fix-public-callers-visibility branch October 29, 2025 13:58


		import static java.util.Collections.emptySet;

		public class AccessibleJdkMethods {

	if (entry.getKey().clazz.startsWith("com/sun/") && entry.getKey().clazz.contains("/internal/")) {
	// skip com.sun.*.internal classes as they are not part of the supported JDK API
	// even if methods override some publicly visible API
	return false;
	}

Fix method visibility in public-callers-finder #137200

Fix method visibility in public-callers-finder #137200

Uh oh!

Conversation

mosche commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mosche commented Oct 27, 2025 •

edited

Loading