Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Restrict classloader for Maven 4 plugins #1336

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Restrict classloader for Maven 4 plugins #1336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions maven-core/src/main/java/org/apache/maven/classrealm/ClassRealmManager.java
View file
Open in desktop
Expand Up @@ -48,6 +48,13 @@ public interface ClassRealmManager {
*/
ClassRealm getMavenApiRealm();

/**
* Gets the class realm exposing the Maven 4 API. This is basically a restricted view on the Maven core realm.
*
* @return The class realm exposing the Maven API, never {@code null}.
*/
ClassRealm getMaven4ApiRealm();

/**
* Creates a new class realm for the specified project and its build extensions.
*
Expand Down
40 changes: 29 additions & 11 deletions maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java
View file
Open in desktop
Expand Up @@ -24,14 +24,8 @@

import java.io.File;
import java.net.MalformedURLException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Random;
import java.util.Set;
import java.util.TreeMap;
import java.util.*;
import java.util.stream.Collectors;

import org.apache.maven.artifact.ArtifactUtils;
import org.apache.maven.classrealm.ClassRealmRequest.RealmType;
Expand All @@ -58,6 +52,8 @@
public class DefaultClassRealmManager implements ClassRealmManager {
public static final String API_REALMID = "maven.api";

public static final String API_V4_REALMID = "maven.api.v4";

/**
* During normal command line build, ClassWorld is loaded by jvm system classloader, which only includes
* plexus-classworlds jar and possibly javaagent classes, see https://issues.apache.org/jira/browse/MNG-4747.
Expand All @@ -79,12 +75,16 @@ public class DefaultClassRealmManager implements ClassRealmManager {

private final ClassRealm mavenApiRealm;

private final ClassRealm maven4ApiRealm;

/**
* Patterns of artifacts provided by maven core and exported via maven api realm. These artifacts are filtered from
* plugin and build extensions realms to avoid presence of duplicate and possibly conflicting classes on classpath.
*/
private final Set<String> providedArtifacts;

private final Set<String> providedArtifactsV4;

@Inject
public DefaultClassRealmManager(
PlexusContainer container, List<ClassRealmManagerDelegate> delegates, CoreExports exports) {
Expand All @@ -102,7 +102,18 @@ public DefaultClassRealmManager(
foreignImports,
null /* artifacts */);

Map<String, ClassLoader> apiV4Imports = new HashMap<>();
apiV4Imports.put("org.apache.maven.api", containerRealm);
apiV4Imports.put("org.slf4j", containerRealm);
apiV4Imports.put("jakarta.inject.*", containerRealm);
this.maven4ApiRealm = createRealm(API_V4_REALMID, RealmType.Core, null, null, apiV4Imports, null);

this.providedArtifacts = exports.getExportedArtifacts();

this.providedArtifactsV4 = providedArtifacts.stream()
.filter(ga ->
ga.startsWith("org.apache.maven:maven-api-") || ga.equals("jakarta.inject:jakarta.inject-api"))
.collect(Collectors.toSet());
}

private ClassRealm newRealm(String id) {
Expand All @@ -129,6 +140,11 @@ public ClassRealm getMavenApiRealm() {
return mavenApiRealm;
}

@Override
public ClassRealm getMaven4ApiRealm() {
return maven4ApiRealm;
}

/**
* Creates a new class realm with the specified parent and imports.
*
Expand All @@ -151,8 +167,9 @@ private ClassRealm createRealm(
List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());

if (artifacts != null && !artifacts.isEmpty()) {
boolean v4api = foreignImports != null && foreignImports.containsValue(maven4ApiRealm);
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact) && artifact.getFile() != null) {
if (!isProvidedArtifact(artifact, v4api) && artifact.getFile() != null) {
constituents.add(new ArtifactClassRealmConstituent(artifact));
} else if (logger.isDebugEnabled()) {
logger.debug(" Excluded: {}", getId(artifact));
Expand Down Expand Up @@ -212,8 +229,9 @@ public ClassRealm createExtensionRealm(Plugin plugin, List<Artifact> artifacts)
getKey(plugin, true), RealmType.Extension, PARENT_CLASSLOADER, null, foreignImports, artifacts);
}

private boolean isProvidedArtifact(Artifact artifact) {
return providedArtifacts.contains(artifact.getGroupId() + ":" + artifact.getArtifactId());
private boolean isProvidedArtifact(Artifact artifact, boolean v4api) {
Set<String> provided = v4api ? providedArtifactsV4 : providedArtifacts;
return provided.contains(artifact.getGroupId() + ":" + artifact.getArtifactId());
}

public ClassRealm createPluginRealm(
Expand Down
9 changes: 6 additions & 3 deletions maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
View file
Open in desktop
Expand Up @@ -348,7 +348,8 @@ public void setupPluginRealm(
pluginDescriptor.setClassRealm(pluginRealm);
pluginDescriptor.setArtifacts(pluginArtifacts);
} else {
Map<String, ClassLoader> foreignImports = calcImports(project, parent, imports);
boolean v4api = pluginDescriptor.getMojos().stream().anyMatch(MojoDescriptor::isV4Api);
Map<String, ClassLoader> foreignImports = calcImports(project, parent, imports, v4api);

PluginRealmCache.Key cacheKey = pluginRealmCache.createKey(
plugin,
Expand Down Expand Up @@ -480,14 +481,16 @@ private List<Artifact> toMavenArtifacts(DependencyNode root, PreorderNodeListGen
return Collections.unmodifiableList(artifacts);
}

private Map<String, ClassLoader> calcImports(MavenProject project, ClassLoader parent, List<String> imports) {
private Map<String, ClassLoader> calcImports(
MavenProject project, ClassLoader parent, List<String> imports, boolean v4api) {
Map<String, ClassLoader> foreignImports = new HashMap<>();

ClassLoader projectRealm = project.getClassRealm();
if (projectRealm != null) {
foreignImports.put("", projectRealm);
} else {
foreignImports.put("", classRealmManager.getMavenApiRealm());
foreignImports.put(
"", v4api ? classRealmManager.getMaven4ApiRealm() : classRealmManager.getMavenApiRealm());
}

if (parent != null && imports != null) {
Expand Down
3 changes: 3 additions & 0 deletions maven-core/src/main/resources/META-INF/maven/extension.xml
View file
Open in desktop
Expand Up @@ -143,7 +143,9 @@ under the License.
<exportedArtifact>org.apache.maven:maven-api-core</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-meta</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-model</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-plugin</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-settings</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-spi</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-toolchain</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-xml</exportedArtifact>

Expand Down Expand Up @@ -187,6 +189,7 @@ under the License.
<exportedArtifact>org.apache.maven.resolver:maven-resolver-util</exportedArtifact>
<exportedArtifact>org.apache.maven.resolver:maven-resolver-connector-basic</exportedArtifact>

<exportedArtifact>jakarta.inject:jakarta.inject-api</exportedArtifact>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think it should land there, ideally we shouldn't have it (we saw that javax was an issue already) but if imposed by some part of the stack (not sure which one since I thought we stayed on javax intentionally) we should just hide it from any consumer IMHO

Copy link
Contributor Author

@gnodet gnodet Dec 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't. We need it in the new api. It's used to define scopes (those can't work without the package) and it's also used to inject various components.
See #1309

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you mean not adding it for Maven 3.x plugins ? That would be absolutely fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not adding at all to anything, will bite us for sure, my understanding was that the guice upgrade was done under javax umbrella and/or we facade any needed api. Ad of today using javax.inject is fine cause code is frozen but anything jakarta can break without notice and can conflict with mojo so goes against maven4 api track for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other solution I could think about is to rewrite the whole DI injection (Sisu + Guice) because they are not pluggable at all. I'm not ready for that...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I don't see why:

  • injections in mojo are already bridged
  • abstracting injection markers (with a maven inject annotation) is easy in guice
  • abstracting the injector or a lookup (like we had/have in plexus container) is quite trivial
  • abstracting a scope is not crazy (wayless than what we already have)

So overall, even if I'm not sure which case(s) you want to cover, I don't see a reason to break our original hypotheis to define a clean and maven owned API and export anything jakarta related.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've raised https://issues.apache.org/jira/browse/MNG-7954

<exportedArtifact>javax.inject:javax.inject</exportedArtifact>
<exportedArtifact>javax.annotation:javax.annotation-api</exportedArtifact>
<exportedArtifact>org.slf4j:slf4j-api</exportedArtifact>
Expand Down