Enhance class name validation and adjust JaCoCo coverage thresholds

Author: peter-lawrey

src/main/docs/decision-log.adoc

@@ -0,0 +1,37 @@
+=== [RC-FN-001] Allow hyphenated descriptor class names
+
+* Date: 2025-10-28
+* Context:
+** The runtime compiler recently introduced stricter validation that rejected binary names containing hyphens.
+** Java reserves `module-info` and `package-info` descriptors, and downstream uses rely on compiling them through the cached compiler.
+** We must prevent injection of directory traversal or shell-sensitive characters while honouring legitimate descriptor forms.
+* Decision Statement:
+** Relax the class name validation to accept hyphenated segments such as `module-info` and `package-info`, while maintaining segment level controls for other characters.
+* Notes/Links:
+** Change implemented in `src/main/java/net/openhft/compiler/CachedCompiler.java`.
+
+=== [RC-TEST-002] Align coverage gate with achieved baseline
+
+* Date: 2025-10-28
+* Context:
+** The enforced JaCoCo minimums were 83 % line and 76 % branch coverage, below both the documentation target and the current test suite capability.
+** Recent test additions raise the baseline to ~85 % line and branch coverage, but still fall short of the historical 90 % goal.
+** Failing builds on the higher 90 % target blocks releases without immediate scope to add more tests.
+* Decision Statement:
+** Increase the JaCoCo enforcement thresholds to 85 % for line and branch coverage so the build reflects the present safety net while keeping headroom for future improvements.
+* *Alternatives Considered:*
+** Retain the 90 % requirement:
+*** _Pros:_ Preserves the original aspiration.
+*** _Cons:_ The build fails despite the current suite, causing friction for ongoing work.
+** Keep legacy 83/76 % thresholds:
+*** _Pros:_ No configuration change needed.
+*** _Cons:_ Enforcement would lag the actual quality level, risking future regressions.
+* *Rationale for Decision:*
+** Setting the guard at 85 % matches the measurable baseline and ensures regression detection without blocking releases.
+** The documentation and configuration now stay consistent, supporting future increments once more tests land.
+* *Impact & Consequences:*
+** Build pipelines now fail if coverage slips below the new 85 % thresholds.
+** Documentation for requirement JRC-TEST-014 is updated to the same value.
+* Notes/Links:
+** Thresholds maintained in `pom.xml`.
+** Updated requirement: `src/main/docs/project-requirements.adoc`.

src/main/java/net/openhft/compiler/CachedCompiler.java

@@ -12,15 +12,15 @@
 import javax.tools.DiagnosticListener;
 import javax.tools.JavaFileObject;
 import javax.tools.StandardJavaFileManager;
-import java.io.Closeable;
-import java.io.File;
-import java.io.IOException;
-import java.io.PrintWriter;
+import java.io.*;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.regex.Pattern;
 
 import static net.openhft.compiler.CompilerUtils.*;
 
@@ -39,18 +39,25 @@ public class CachedCompiler implements Closeable {
     /**
      * Writer used when no alternative is supplied.
      */
-    private static final PrintWriter DEFAULT_WRITER = new PrintWriter(System.err);
+    private static final PrintWriter DEFAULT_WRITER = createDefaultWriter();
     /**
      * Default compiler flags including debug symbols.
      */
     private static final List<String> DEFAULT_OPTIONS = Arrays.asList("-g", "-nowarn");
+    private static final Pattern CLASS_NAME_PATTERN = Pattern.compile("[\\p{Alnum}_$.\\-]+");
+    private static final Pattern CLASS_NAME_SEGMENT_PATTERN = Pattern.compile("[\\p{Alnum}_$]+(?:-[\\p{Alnum}_$]+)*");
 
     private final Map<ClassLoader, Map<String, Class<?>>> loadedClassesMap = Collections.synchronizedMap(new WeakHashMap<>());
     private final Map<ClassLoader, MyJavaFileManager> fileManagerMap = Collections.synchronizedMap(new WeakHashMap<>());
     /**
      * Optional testing hook to replace the file manager implementation.
+     * <p>
+     * This field remains {@code public} to preserve binary compatibility with callers that
+     * accessed it directly in previous releases. Prefer {@link #setFileManagerOverride(Function)}
+     * for source-compatible code.
      */
-    public Function<StandardJavaFileManager, MyJavaFileManager> fileManagerOverride;
+    @SuppressWarnings("WeakerAccess")
+    public volatile Function<StandardJavaFileManager, MyJavaFileManager> fileManagerOverride;
 
     @Nullable
     private final File sourceDir;
@@ -84,7 +91,7 @@ public CachedCompiler(@Nullable File sourceDir,
                           @NotNull List<String> options) {
         this.sourceDir = sourceDir;
         this.classDir = classDir;
-        this.options = options;
+        this.options = Collections.unmodifiableList(new ArrayList<>(options));
     }
 
     /**
@@ -111,6 +118,7 @@ public void close() {
      * @throws ClassNotFoundException if the compiled class cannot be defined
      */
     public Class<?> loadFromJava(@NotNull String className, @NotNull String javaCode) throws ClassNotFoundException {
+        validateClassName(className);
         return loadFromJava(getClass().getClassLoader(), className, javaCode, DEFAULT_WRITER);
     }
 
@@ -127,6 +135,7 @@ public Class<?> loadFromJava(@NotNull String className, @NotNull String javaCode
     public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
                                  @NotNull String className,
                                  @NotNull String javaCode) throws ClassNotFoundException {
+        validateClassName(className);
         return loadFromJava(classLoader, className, javaCode, DEFAULT_WRITER);
     }
 
@@ -144,6 +153,7 @@ public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
     Map<String, byte[]> compileFromJava(@NotNull String className,
                                         @NotNull String javaCode,
                                         MyJavaFileManager fileManager) {
+        validateClassName(className);
         return compileFromJava(className, javaCode, DEFAULT_WRITER, fileManager);
     }
 
@@ -162,10 +172,11 @@ Map<String, byte[]> compileFromJava(@NotNull String className,
                                         @NotNull String javaCode,
                                         final @NotNull PrintWriter writer,
                                         MyJavaFileManager fileManager) {
+        validateClassName(className);
         Iterable<? extends JavaFileObject> compilationUnits;
         if (sourceDir != null) {
             String filename = className.replaceAll("\\.", '\\' + File.separator) + ".java";
-            File file = new File(sourceDir, filename);
+            File file = safeResolve(sourceDir, filename);
             writeText(file, javaCode);
             if (s_standardJavaFileManager == null)
                 s_standardJavaFileManager = s_compiler.getStandardFileManager(null, null, null);
@@ -199,6 +210,7 @@ public void report(Diagnostic<? extends JavaFileObject> diagnostic) {
         }
     }
 
+
     /**
      * Compile and load using a specific class loader and writer. The
      * compilation result is cached against the loader for future calls.
@@ -223,7 +235,7 @@ public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
             else
                 clazz = loadedClasses.get(className);
         }
-        PrintWriter printWriter = (writer == null ? DEFAULT_WRITER : writer);
+        PrintWriter printWriter = writer == null ? DEFAULT_WRITER : writer;
         if (clazz != null)
             return clazz;
 
@@ -236,14 +248,15 @@ public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
         final Map<String, byte[]> compiled = compileFromJava(className, javaCode, printWriter, fileManager);
         for (Map.Entry<String, byte[]> entry : compiled.entrySet()) {
             String className2 = entry.getKey();
+            validateClassName(className2);
             synchronized (loadedClassesMap) {
                 if (loadedClasses.containsKey(className2))
                     continue;
             }
             byte[] bytes = entry.getValue();
             if (classDir != null) {
                 String filename = className2.replaceAll("\\.", '\\' + File.separator) + ".class";
-                boolean changed = writeBytes(new File(classDir, filename), bytes);
+                boolean changed = writeBytes(safeResolve(classDir, filename), bytes);
                 if (changed) {
                     LOG.info("Updated {} in {}", className2, classDir);
                 }
@@ -281,9 +294,46 @@ public void updateFileManagerForClassLoader(ClassLoader classLoader, Consumer<My
         }
     }
 
+    public void setFileManagerOverride(Function<StandardJavaFileManager, MyJavaFileManager> fileManagerOverride) {
+        this.fileManagerOverride = fileManagerOverride;
+    }
+
     private @NotNull MyJavaFileManager getFileManager(StandardJavaFileManager fm) {
         return fileManagerOverride != null
                 ? fileManagerOverride.apply(fm)
                 : new MyJavaFileManager(fm);
     }
+
+    private static void validateClassName(String className) {
+        Objects.requireNonNull(className, "className");
+        if (!CLASS_NAME_PATTERN.matcher(className).matches()) {
+            throw new IllegalArgumentException("Invalid class name: " + className);
+        }
+        for (String segment : className.split("\\.", -1)) {
+            if (!CLASS_NAME_SEGMENT_PATTERN.matcher(segment).matches()) {
+                throw new IllegalArgumentException("Invalid class name: " + className);
+            }
+        }
+    }
+
+    static File safeResolve(File root, String relativePath) {
+        Objects.requireNonNull(root, "root");
+        Objects.requireNonNull(relativePath, "relativePath");
+        Path base = root.toPath().toAbsolutePath().normalize();
+        Path candidate = base.resolve(relativePath).normalize();
+        if (!candidate.startsWith(base)) {
+            throw new IllegalArgumentException("Attempted path traversal for " + relativePath);
+        }
+        return candidate.toFile();
+    }
+
+    private static PrintWriter createDefaultWriter() {
+        OutputStreamWriter writer = new OutputStreamWriter(System.err, StandardCharsets.UTF_8);
+        return new PrintWriter(writer, true) {
+            @Override
+            public void close() {
+                flush(); // never close System.err
+            }
+        };
+    }
 }

src/main/java/net/openhft/compiler/CompilerUtils.java

@@ -19,7 +19,9 @@
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.nio.charset.Charset;
+import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.Objects;
 
 /**
  * Provides static utility methods for runtime Java compilation, dynamic class loading,
@@ -148,6 +150,25 @@ public static boolean addClassPath(@NotNull String dir) {
         return true;
     }
 
+    /**
+     * Normalizes relative paths and rejects traversal attempts beyond the current root.
+     *
+     * @param path value to sanitize.
+     * @return normalized path when safe.
+     * @throws IllegalArgumentException if the path attempts to traverse upward ("..").
+     */
+    static Path sanitizePath(@NotNull Path path) {
+        Objects.requireNonNull(path, "path");
+        Path normalized = path.normalize();
+        if (normalized.isAbsolute()) {
+            return normalized;
+        }
+        if (normalized.getNameCount() > 0 && "..".equals(normalized.getName(0).toString())) {
+            throw new IllegalArgumentException("Path traversal attempt: " + path);
+        }
+        return normalized;
+    }
+
     /**
      * Define a class for byte code.
      *
@@ -303,6 +324,12 @@ public static boolean writeBytes(@NotNull File file, @NotNull byte[] bytes) {
             if (bak != null)
                 bak.renameTo(file);
             throw new IllegalStateException("Unable to write " + file, e);
+        } finally {
+            if (bak != null && bak.exists() && file.exists()) {
+                if (!bak.delete()) {
+                    LOGGER.debug("Unable to delete backup {}", bak);
+                }
+            }
         }
         return true;
     }