From 56368519a242de0a8b5323fc6fc5d3e02c5dfb16 Mon Sep 17 00:00:00 2001 From: "Henning P. Schmiedehausen" Date: Sat, 14 Sep 2024 16:51:37 -0700 Subject: [PATCH] Fix endless loop with floating relocation pattern Fixes #4. Thank you @jkebinger for reporting! --- pom.xml | 29 ++++++- ...CollectingManifestResourceTransformer.java | 77 ++++++++++--------- ...ectingManifestResourceTransformerTest.java | 62 +++++++++++++++ 3 files changed, 129 insertions(+), 39 deletions(-) create mode 100644 src/test/java/org/basepom/maven/shade/CollectingManifestResourceTransformerTest.java diff --git a/pom.xml b/pom.xml index cba27ba..583c0b0 100644 --- a/pom.xml +++ b/pom.xml @@ -12,7 +12,8 @@ ~ See the License for the specific language governing permissions and ~ limitations under the License. --> - + 4.0.0 @@ -61,8 +62,22 @@ 11 org.basepom.policy true + + 5.11.0 + + + + org.junit + junit-bom + ${dep.junit5.version} + pom + import + + + + org.apache.maven.plugins @@ -83,5 +98,17 @@ ${dep.spotbugs.version} true + + + org.junit.jupiter + junit-jupiter-api + test + + + + org.junit.jupiter + junit-jupiter-engine + test + diff --git a/src/main/java/org/basepom/maven/shade/CollectingManifestResourceTransformer.java b/src/main/java/org/basepom/maven/shade/CollectingManifestResourceTransformer.java index 5c83887..dd9998d 100644 --- a/src/main/java/org/basepom/maven/shade/CollectingManifestResourceTransformer.java +++ b/src/main/java/org/basepom/maven/shade/CollectingManifestResourceTransformer.java @@ -11,8 +11,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package org.basepom.maven.shade; +import static java.lang.String.format; + import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; @@ -26,17 +29,17 @@ import java.util.jar.JarFile; import java.util.jar.JarOutputStream; import java.util.jar.Manifest; + import org.apache.maven.plugins.shade.relocation.Relocator; import org.apache.maven.plugins.shade.resource.ManifestResourceTransformer; /** - * Extends {@link ManifestResourceTransformer} to collect the additional sections - * in the jar manifests. This keeps the build information from the internal jars in the - * shaded jars. Submitted back to Maven as MSHADE-165. + * Extends {@link ManifestResourceTransformer} to collect the additional sections in the jar manifests. This keeps the build information from the internal jars + * in the shaded jars. Submitted back to Maven as MSHADE-165. */ public final class CollectingManifestResourceTransformer - extends ManifestResourceTransformer -{ + extends ManifestResourceTransformer { + private final List defaultAttributes = Arrays.asList("Export-Package", "Import-Package", "Provide-Capability", @@ -56,22 +59,19 @@ public final class CollectingManifestResourceTransformer private long time = Long.MIN_VALUE; /** - * If set, the transformer will collect all sections from jar manifests - * and adds them to the main manifest. + * If set, the transformer will collect all sections from jar manifests and adds them to the main manifest. */ public void setCollectSections(boolean collectSections) { this.collectSections = collectSections; } @Override - public void setMainClass( String mainClass ) - { + public void setMainClass(String mainClass) { this.mainClass = mainClass; } @Override - public void setManifestEntries(Map manifestEntries) - { + public void setManifestEntries(Map manifestEntries) { if (manifestEntries != null) { this.manifestEntries = new HashMap<>(manifestEntries); } else { @@ -80,8 +80,7 @@ public void setManifestEntries(Map manifestEntries) } @Override - public void setAdditionalAttributes(List additionalAttributes) - { + public void setAdditionalAttributes(List additionalAttributes) { if (additionalAttributes != null) { this.additionalAttributes = new ArrayList<>(additionalAttributes); } else { @@ -90,15 +89,13 @@ public void setAdditionalAttributes(List additionalAttributes) } @Override - public boolean canTransformResource(String resource) - { + public boolean canTransformResource(String resource) { return JarFile.MANIFEST_NAME.equalsIgnoreCase(resource); } @Override public void processResource(String resource, InputStream is, List relocators, long time) - throws IOException - { + throws IOException { Manifest loadedManifest = new Manifest(is); // Relocate the just loaded manifest @@ -141,33 +138,25 @@ public void processResource(String resource, InputStream is, List rel // loop through the sections of the just loaded manifest loadedManifest.getEntries().forEach((key, value) -> { - // Find the section that matches this entry or create a new one. - Attributes existingRootSectionAttributes = existingRootSections.get(key); - // create a new section if it does not already exist - if (existingRootSectionAttributes == null) { - existingRootSectionAttributes = new Attributes(); - existingRootSections.put(key, existingRootSectionAttributes); - } + Attributes existingRootSectionAttributes = existingRootSections.computeIfAbsent(key, k -> new Attributes()); // Add all attributes from that section to the manifest if (value != null) { - value.forEach(existingRootSectionAttributes::put); + existingRootSectionAttributes.putAll(value); } }); } } @Override - public boolean hasTransformedResource() - { + public boolean hasTransformedResource() { return true; } @Override public void modifyOutputStream(JarOutputStream jos) - throws IOException - { + throws IOException { // If we didn't find a manifest, then let's create one. if (rootManifest == null) { rootManifest = new Manifest(); @@ -193,17 +182,29 @@ public void modifyOutputStream(JarOutputStream jos) rootManifest.write(jos); } - private String relocate(String originalValue, List relocators) - { - String newValue = originalValue; + static String relocate(String originalName, List relocators) { + String existingPrefix = null; + String relocatedName = null; for (Relocator relocator : relocators) { - String value; - do { - value = newValue; - newValue = relocator.relocateClass(value); + for (; ; ) { + relocatedName = relocator.relocateClass(originalName); + if (originalName.equals(relocatedName)) { + break; // for(;; + } + // see if this is just a regular "add a prefix" relocation + if (relocatedName.endsWith(originalName)) { + String prefix = relocatedName.substring(0, relocatedName.length() - originalName.length()); + if (prefix.equals(existingPrefix)) { + String errorMessage = format("Detected loop when relocating %s to %s, prefix %s already applied!%nRelocation pattern () may be unanchored (not starting with '^')!", originalName, + relocatedName, prefix); + throw new IllegalStateException(errorMessage); + } + existingPrefix = prefix; + } + + originalName = relocatedName; } - while (!value.equals(newValue)); } - return newValue; + return relocatedName; } } diff --git a/src/test/java/org/basepom/maven/shade/CollectingManifestResourceTransformerTest.java b/src/test/java/org/basepom/maven/shade/CollectingManifestResourceTransformerTest.java new file mode 100644 index 0000000..7e714d7 --- /dev/null +++ b/src/test/java/org/basepom/maven/shade/CollectingManifestResourceTransformerTest.java @@ -0,0 +1,62 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.basepom.maven.shade; + +import java.util.Collections; + +import org.apache.maven.plugins.shade.relocation.Relocator; +import org.apache.maven.plugins.shade.relocation.SimpleRelocator; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +class CollectingManifestResourceTransformerTest { + + @Test + void testPatternRelocation() { + Relocator relocator = new SimpleRelocator("^com\\.google\\.common", "prefab.shaded.com.google.common", null, null); + + Assertions.assertEquals("prefab.shaded.com.google.common.SomeClass", + CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator))); + } + + @Test + void testAnchoredRelocation() { + Relocator relocator = new SimpleRelocator("^com.google.common", "prefab.shaded.com.google.common", null, null); + + Assertions.assertEquals("prefab.shaded.com.google.common.SomeClass", + CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator))); + } + + @Test + void testFloatingNonMatchingRelocation() { + Relocator relocator = new SimpleRelocator("com.google.common", "prefab.shaded.cgm", null, null); + + Assertions.assertEquals("prefab.shaded.cgm.SomeClass", + CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator))); + } + + @Test + void testFloatingMatchingRelocation() { + Relocator relocator = new SimpleRelocator("com.google.common", "prefab.shaded.com.google.common", null, null); + + IllegalStateException exception = Assertions.assertThrows(IllegalStateException.class, + () -> CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator))); + + Assertions.assertEquals( + "Detected loop when relocating prefab.shaded.com.google.common.SomeClass to prefab.shaded.prefab.shaded.com.google.common.SomeClass, " + + "prefix prefab.shaded. already applied!\nRelocation pattern () may be unanchored (not starting with '^')!", + exception.getMessage()); + } +}