fix(common): strip leading whitespace in ClassPreLoader (#2560)

* fix(common): strip leading whitespace in ClassPreLoader

ClassPreLoader used line.stripTrailing() when parsing classesloaded.txt,
which keeps leading whitespace. Modules that ship the file with a leading
space on every line (for example powertools-tracing) passed names such as
" java.lang.Object" to Class.forName, which throws ClassNotFoundException,
so SnapStart class priming loaded 0 classes.

Use line.strip() so both leading and trailing whitespace are removed. Add a
regression test that references a class with a leading space.

Closes #2559

* fix(common): reject malformed classesloaded.txt entries and fix tracing test isolation

Harden ClassPreLoader to only pass well-formed binary class names to
Class.forName. Some modules ship classesloaded.txt with malformed entries
such as runtime-synthetic lambda classes and lines that glue a class name to
a file path, none of which are loadable. Add a debug log reporting how many
classes were preloaded.

Fixing the leading-whitespace parsing means the well-formed classes in
powertools-tracing now actually load during priming. That exposed a
pre-existing test isolation defect: PowerTracerToolEnabledForResponseWithCustomMapper
set the static TracingUtils.objectMapper from a static initializer, which
leaked into later tests in the same JVM fork and changed how errors were
serialized. Set the mapper from the constructor instead and reset it in the
aspect test teardown.

Relates to #2559

* fix(common): simplify class name pattern to avoid backtracking

Replace the nested-quantifier regex with a single linear character class.
SonarCloud flagged the previous pattern for possible catastrophic
backtracking on large inputs. The character class rejects the same malformed
entries (paths, URL-encoded spaces, synthetic lambda names) without the
backtracking risk.

* refactor(common): drop class name pattern, rely on Class.forName

Remove the binary class name pattern matching. Class.forName already rejects
malformed entries by throwing ClassNotFoundException, which is caught and
ignored, so the extra pattern check only duplicated that rejection and added
complexity. The tracing test failures were caused by test ordering (a leaked
static ObjectMapper), not by malformed entries, and that fix is retained.

Author: phipag

powertools-common/src/main/java/software/amazon/lambda/powertools/common/internal/ClassPreLoader.java

@@ -22,12 +22,17 @@
 import java.net.URLConnection;
 import java.util.Enumeration;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Used to preload classes to support automatic priming for SnapStart
  */
 public final class ClassPreLoader {
     public static final String CLASSES_FILE = "classesloaded.txt";
 
+    private static final Logger LOG = LoggerFactory.getLogger(ClassPreLoader.class);
+
     private ClassPreLoader() {
         // Hide default constructor
     }
@@ -57,6 +62,7 @@ public static void preloadClasses() {
      * @param is
      */
     private static void preloadClassesFromStream(InputStream is) {
+        int loaded = 0;
         try (is;
              InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8);
              BufferedReader reader = new BufferedReader(isr)) {
@@ -66,26 +72,30 @@ private static void preloadClassesFromStream(InputStream is) {
                 if (idx != -1) {
                     line = line.substring(0, idx);
                 }
-                final String className = line.stripTrailing();
-                if (!className.isBlank()) {
-                    loadClassIfFound(className);
+                final String className = line.strip();
+                if (!className.isBlank() && loadClassIfFound(className)) {
+                    loaded++;
                 }
             }
         } catch (Exception ignored) {
             // No action is required if preloading fails for any reason
         }
+        LOG.debug("SnapStart priming: preloaded {} class(es) from {}", loaded, CLASSES_FILE);
     }
 
     /**
      * Initializes the class with given name if found, ignores otherwise
      *
-     * @param className
+     * @param className the binary name of the class to load
+     * @return true if the class was found and loaded, false otherwise
      */
-    private static void loadClassIfFound(String className) {
+    private static boolean loadClassIfFound(String className) {
         try {
             Class.forName(className, true, ClassPreLoader.class.getClassLoader());
+            return true;
         } catch (ClassNotFoundException e) {
             // No action is required if the class with given name cannot be found
+            return false;
         }
     }
 }

powertools-common/src/test/java/software/amazon/lambda/powertools/common/internal/ClassPreLoaderTest.java

@@ -8,6 +8,7 @@ class ClassPreLoaderTest {
 
     // Making this volatile so the Thread Context doesn't need any special handling
     static volatile boolean dummyClassLoaded = false;
+    static volatile boolean leadingSpaceDummyClassLoaded = false;
 
     /**
      * Dummy class to be loaded by ClassPreLoader in test.
@@ -20,15 +21,35 @@ static class DummyClass {
             dummyClassLoaded = true;
         }
     }
+
+    /**
+     * Dummy class whose name is referenced with a leading space in
+     * <i>powertools-common/src/test/resources/classesloaded.txt</i>.
+     * Some modules ship a classesloaded.txt with a leading space on every line, so the loader must
+     * strip leading whitespace before calling Class.forName.
+     */
+    static class LeadingSpaceDummyClass {
+        static {
+            leadingSpaceDummyClassLoaded = true;
+        }
+    }
+
     @Test
     void preloadClasses_shouldIgnoreInvalidClassesAndLoadValidClasses() {
 
         dummyClassLoaded = false;
-        // powertools-common/src/test/resources/classesloaded.txt has a class that does not exist
+        leadingSpaceDummyClassLoaded = false;
+        // A class only runs its static initializer the first time it is loaded, so this test calls
+        // preloadClasses once and asserts on all classes listed in classesloaded.txt together.
+        // powertools-common/src/test/resources/classesloaded.txt has a class that does not exist.
         // Verify that the missing class did not throw any exception
         assertDoesNotThrow(ClassPreLoader::preloadClasses);
 
-        // When the classloaded.txt is a mixed bag of valid and invalid classes, Valid class must load
+        // When the classloaded.txt is a mixed bag of valid and invalid classes, valid classes must load
         assertTrue(dummyClassLoaded, "DummyClass should be loaded");
+        // classesloaded.txt references LeadingSpaceDummyClass with a leading space. The loader must
+        // strip it before Class.forName, otherwise the class is never loaded.
+        assertTrue(leadingSpaceDummyClassLoaded,
+                "LeadingSpaceDummyClass should be loaded despite the leading space");
     }
 }

powertools-common/src/test/resources/classesloaded.txt

@@ -1,2 +1,3 @@
 software.amazon.lambda.powertools.common.internal.NonExistingClass
-software.amazon.lambda.powertools.common.internal.ClassPreLoaderTest$DummyClass
+software.amazon.lambda.powertools.common.internal.ClassPreLoaderTest$DummyClass
+ software.amazon.lambda.powertools.common.internal.ClassPreLoaderTest$LeadingSpaceDummyClass

powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/handlers/PowerTracerToolEnabledForResponseWithCustomMapper.java

@@ -28,7 +28,11 @@
 import software.amazon.lambda.powertools.tracing.TracingUtils;
 
 public class PowerTracerToolEnabledForResponseWithCustomMapper implements RequestHandler<Object, Object> {
-    static {
+    public PowerTracerToolEnabledForResponseWithCustomMapper() {
+        // Set the custom mapper in the constructor rather than a static initializer. A static
+        // initializer only runs the first time this class is loaded, which makes the resulting
+        // global state depend on when the class happens to be loaded (for example by SnapStart
+        // class priming). Setting it in the constructor makes each instantiation deterministic.
         ObjectMapper objectMapper = new ObjectMapper();
         SimpleModule simpleModule = new SimpleModule();
         simpleModule.addSerializer(ChildClass.class, new ChildSerializer());

powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/internal/LambdaTracingAspectTest.java

@@ -41,6 +41,7 @@
 import software.amazon.lambda.powertools.tracing.handlers.PowerTracerToolEnabledExplicitlyForResponseAndError;
 import software.amazon.lambda.powertools.tracing.handlers.PowerTracerToolEnabledForError;
 import software.amazon.lambda.powertools.tracing.handlers.PowerTracerToolEnabledForResponse;
+import software.amazon.lambda.powertools.tracing.TracingUtils;
 import software.amazon.lambda.powertools.tracing.handlers.PowerTracerToolEnabledForResponseWithCustomMapper;
 import software.amazon.lambda.powertools.tracing.handlers.PowerTracerToolEnabledForStream;
 import software.amazon.lambda.powertools.tracing.handlers.PowerTracerToolEnabledForStreamWithNoMetaData;
@@ -70,6 +71,10 @@ void setUp() throws IllegalAccessException {
     @AfterEach
     void tearDown() {
         AWSXRay.endSegment();
+        // Some tests (e.g. the custom mapper handler) set the static TracingUtils.objectMapper.
+        // Reset it so it does not leak into later tests running in the same JVM fork and change how
+        // responses and errors are serialized.
+        TracingUtils.defaultObjectMapper(null);
     }
 
     @Test