gnodet-bot commented on code in PR #13570:
URL: https://github.com/apache/maven/pull/13570#discussion_r4295512197
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -135,5 +144,48 @@ public Optional<T> lookup(String id) {
}
return Optional.ofNullable(values.get(key));
}
+
+ /**
+ * Checks whether a provider is visible from the given classloader.
+ * Providers from project extensions are loaded by extension-specific
classloaders
+ * (realms) that are only imported into the project realm that
declared the extension.
+ */
+ private boolean isVisibleFrom(P provider, ClassLoader classLoader) {
+ if (classLoader == null) {
+ return true;
+ }
+ Class<?> providerClass = provider.getClass();
+ ClassLoader providerCl = providerClass.getClassLoader();
+ if (providerCl == null) {
+ return true;
+ }
+ // Check loadClass — works for standard classloaders and
ClassRealm with package-level exports
+ try {
+ if (classLoader.loadClass(providerClass.getName()) ==
providerClass) {
+ return true;
+ }
+ } catch (ClassNotFoundException e) {
+ // Not loadable by name (hidden class or realm-ID import) —
check further
Review Comment:
⚠️ **Bug:** `ClassLoader.loadClass()` can throw `NoClassDefFoundError` (a
`LinkageError`) when the class was previously loaded but its dependencies
failed to link. Only `ClassNotFoundException` is caught here — a `LinkageError`
escapes unchecked and aborts the entire `lookup()` call.
```suggestion
} catch (ClassNotFoundException | LinkageError e) {
// Not loadable by name (hidden class or realm-ID import) —
check further
}
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -135,5 +144,48 @@ public Optional<T> lookup(String id) {
}
return Optional.ofNullable(values.get(key));
}
+
+ /**
+ * Checks whether a provider is visible from the given classloader.
+ * Providers from project extensions are loaded by extension-specific
classloaders
Review Comment:
🧪 **Missing test coverage:** The new `isVisibleFrom()` / `isImportedRealm()`
logic has zero unit test coverage. The new test class
`DefaultExtensibleEnumRegistryTest` only tests the outer `lookup()` method with
a single classloader — it doesn't exercise:
- A provider whose classloader is `null` (bootstrap)
- A provider from a different classloader that is **not** visible
- A provider from a parent classloader (visible via chain walk)
- The `isImportedRealm()` reflection path (ClassRealm import detection)
Please add dedicated tests for `isVisibleFrom()` covering these cases.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -118,14 +120,21 @@ public DefaultExtensibleEnumRegistry(
* thousands; callers that need to look up the same value repeatedly
in a tight loop should
* cache the result themselves.</p>
*
+ * <p><b>Classloader visibility:</b> providers contributed by project
extensions are only
+ * visible when the current thread's context classloader can load the
provider's class.
+ * This prevents extension-scoped providers from leaking across
reactor modules during
+ * parallel builds.</p>
+ *
* @param id the enum value id, must not be {@code null}
* @return the matching value, or {@code Optional.empty()} if not found
*/
@Override
public Optional<T> lookup(String id) {
String key = requireNonNull(id, "id").toLowerCase(Locale.ROOT);
if (lookup != null && providerType != null) {
+ ClassLoader tccl =
Thread.currentThread().getContextClassLoader();
Optional<T> result = lookup.lookupList(providerType).stream()
+ .filter(p -> isVisibleFrom(p, tccl))
.flatMap(p -> p.provides().stream())
Review Comment:
⚠️ **Incomplete fix:** `lookup.lookupList(providerType)` in `lookup()` is
correctly filtered by `isVisibleFrom()` here, but
`DefaultTypeRegistry.require()` and `DefaultLifecycleRegistry.stream()` call
`lookup.lookupList(TypeProvider.class)` /
`lookup.lookupList(LifecycleProvider.class)` without any classloader visibility
filter. Extension-scoped type and lifecycle providers can still leak across
reactor modules through those code paths. The fix needs to be applied there as
well to be complete.
(Files:
`impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultTypeRegistry.java`,
`impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java`)
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -135,5 +144,48 @@ public Optional<T> lookup(String id) {
}
return Optional.ofNullable(values.get(key));
}
+
+ /**
+ * Checks whether a provider is visible from the given classloader.
+ * Providers from project extensions are loaded by extension-specific
classloaders
+ * (realms) that are only imported into the project realm that
declared the extension.
+ */
+ private boolean isVisibleFrom(P provider, ClassLoader classLoader) {
+ if (classLoader == null) {
+ return true;
+ }
+ Class<?> providerClass = provider.getClass();
+ ClassLoader providerCl = providerClass.getClassLoader();
+ if (providerCl == null) {
+ return true;
+ }
+ // Check loadClass — works for standard classloaders and
ClassRealm with package-level exports
+ try {
+ if (classLoader.loadClass(providerClass.getName()) ==
providerClass) {
+ return true;
+ }
+ } catch (ClassNotFoundException e) {
+ // Not loadable by name (hidden class or realm-ID import) —
check further
+ }
+ // Walk the Java classloader parent chain
+ for (ClassLoader cl = classLoader; cl != null; cl =
cl.getParent()) {
+ if (cl == providerCl) {
+ return true;
+ }
+ }
+ // Check ClassRealm foreign imports via reflection — handles the
common case
+ // where Maven imports extension realms using the realm ID rather
than package names
+ return isImportedRealm(classLoader, providerCl);
+ }
+
+ private static boolean isImportedRealm(ClassLoader classLoader,
ClassLoader providerCl) {
+ try {
+ Method method =
classLoader.getClass().getMethod("getImportRealms");
Review Comment:
💡 **Performance:** `getClass().getMethod("getImportRealms")` is called on
every `isVisibleFrom()` invocation, which happens for every provider during
`lookup()`. Reflective method lookup is relatively expensive. Consider caching
the `Method` object per classloader class (e.g. in a `ClassValue<Method>` or a
`ConcurrentHashMap<Class<?>, Optional<Method>>`) to avoid the overhead on
repeated calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
