diff --git a/src/main/java/de/rub/nds/scanner/core/guideline/GuidelineIO.java b/src/main/java/de/rub/nds/scanner/core/guideline/GuidelineIO.java index 984e7e7a..22023f3f 100644 --- a/src/main/java/de/rub/nds/scanner/core/guideline/GuidelineIO.java +++ b/src/main/java/de/rub/nds/scanner/core/guideline/GuidelineIO.java @@ -89,18 +89,22 @@ private static List listXmlFiles(ClassLoader classLoader, String folder) .forEach(p -> xmlFilePaths.add(folder + "/" + p.getFileName().toString())); } } else if ("jar".equals(protocol)) { - // Running from a jar + // Running from a jar. + // The JarFile returned by getJarFile() is the JVM-wide cached instance from + // JarFileFactory (useCaches=true is the default). It must not be closed here: + // closing it would break any other thread mid-iteration on the same shared + // JarFile with IllegalStateException("zip file closed") (see JDK-8246714). + // The JVM owns the lifecycle of cached JarFiles. JarURLConnection jarConnection = (JarURLConnection) url.openConnection(); - try (JarFile jarFile = jarConnection.getJarFile()) { - Enumeration entries = jarFile.entries(); - while (entries.hasMoreElements()) { - JarEntry entry = entries.nextElement(); - String name = entry.getName(); - if (name.startsWith(folder + "/") - && name.endsWith(".xml") - && !entry.isDirectory()) { - xmlFilePaths.add(name); - } + JarFile jarFile = jarConnection.getJarFile(); + Enumeration entries = jarFile.entries(); + while (entries.hasMoreElements()) { + JarEntry entry = entries.nextElement(); + String name = entry.getName(); + if (name.startsWith(folder + "/") + && name.endsWith(".xml") + && !entry.isDirectory()) { + xmlFilePaths.add(name); } } } else { diff --git a/src/test/java/de/rub/nds/scanner/core/guideline/GuidelineIOConcurrencyTest.java b/src/test/java/de/rub/nds/scanner/core/guideline/GuidelineIOConcurrencyTest.java new file mode 100644 index 00000000..f596a3da --- /dev/null +++ b/src/test/java/de/rub/nds/scanner/core/guideline/GuidelineIOConcurrencyTest.java @@ -0,0 +1,288 @@ +/* + * Scanner Core - A Modular Framework for Probe Definition, Execution, and Result Analysis. + * + * Copyright 2017-2023 Ruhr University Bochum, Paderborn University, Technology Innovation Institute, and Hackmanit GmbH + * + * Licensed under Apache License, Version 2.0 + * http://www.apache.org/licenses/LICENSE-2.0.txt + */ +package de.rub.nds.scanner.core.guideline; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import de.rub.nds.scanner.core.guideline.testutil.IOTestGuidelineCheck; +import de.rub.nds.scanner.core.probe.AnalyzedProperty; +import de.rub.nds.scanner.core.probe.AnalyzedPropertyCategory; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.net.JarURLConnection; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.List; +import java.util.concurrent.*; +import java.util.concurrent.atomic.AtomicReference; +import java.util.jar.JarEntry; +import java.util.jar.JarFile; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Verifies that {@link GuidelineIO#readGuidelines} is safe under concurrent access from JAR + * protocol. + * + *

Root cause (fixed): {@code JarURLConnection} caches {@code JarFile} instances JVM-wide + * via {@code JarFileFactory}. All concurrent callers receive the same {@code JarFile} + * object. The original code wrapped it in a {@code try-with-resources}, so the first thread to + * finish closed the shared instance and other threads mid-iteration got {@code + * IllegalStateException: zip file closed} (JDK ≤17) or {@code FileNotFoundException} (JDK 21). + * + *

Fix: {@code listXmlFiles} no longer closes the {@code JarFile}. The JVM's {@code + * JarFileFactory} owns the lifecycle of cached instances, and closing them from user code is the + * bug. Keeping the cache also preserves performance — reopening the jar per call would be expensive + * in high-throughput scanning (millions of connections). + * + *

Two tests: + * + *

    + *
  1. {@link #testUnderlyingJdkCachingStillCausesErrorWithoutFix} — demonstrates the JDK-level + * behaviour: when {@code useCaches=true} (the default), closing the {@code JarFile} returned + * by {@code getJarFile()} breaks any other thread iterating the same shared instance. This is + * why {@code GuidelineIO.listXmlFiles} must not call {@code close()}. + *
  2. {@link #testConcurrentReadGuidelinesIsCorrectAfterFix} — verifies that all concurrent calls + * to {@link GuidelineIO#readGuidelines} return the correct results with the fix in place. + *
+ */ +class GuidelineIOConcurrencyTest { + + private static final int GUIDELINE_FILE_COUNT = 20; + + private static class TestAnalyzedProperty implements AnalyzedProperty { + private final String name; + + TestAnalyzedProperty(String name) { + this.name = name; + } + + @Override + public String getName() { + return name; + } + + @Override + public AnalyzedPropertyCategory getCategory() { + return new TestPropertyCategory(); + } + } + + private Path buildTestJar(Path jarPath, byte[] xmlBytes, int xmlCount) throws Exception { + try (ZipOutputStream zos = new ZipOutputStream(Files.newOutputStream(jarPath))) { + // Explicit directory entry required for JarURLConnection.connect() to resolve the + // folder URL via getJarEntry("test-guidelines/") without FileNotFoundException + ZipEntry dirEntry = new ZipEntry("test-guidelines/"); + zos.putNextEntry(dirEntry); + zos.closeEntry(); + + for (int i = 0; i < xmlCount; i++) { + ZipEntry entry = new ZipEntry("test-guidelines/guideline" + i + ".xml"); + zos.putNextEntry(entry); + zos.write(xmlBytes); + zos.closeEntry(); + } + } + return jarPath; + } + + // ------------------------------------------------------------------------- + // TEST 1 — Proves the underlying JDK caching issue still exists. + // + // This test operates directly on JarURLConnection with useCaches=true (the + // default). It shows that two connections to the same URL share one JarFile, + // and closing it via one reference breaks the other's iterator. + // + // This is the mechanism that made the original bug possible and justifies + // why GuidelineIO.listXmlFiles must not call close() on the cached JarFile. + // + // Thread A → openConnection (useCaches=true) → getJarFile() → shared JarFile@X + // → entries() → nextElement() OK → barrier-1 + // Thread B → barrier-1 → openConnection (useCaches=true) → getJarFile() → same JarFile@X + // → close() → barrier-2 + // Thread A → barrier-2 → entries.nextElement() → THROWS (zip file closed / FNFE) + // ------------------------------------------------------------------------- + @Test + void testUnderlyingJdkCachingStillCausesErrorWithoutFix(@TempDir File tempDir) + throws Exception { + + GuidelineIO io = new GuidelineIO(TestAnalyzedProperty.class); + Guideline guideline = + new Guideline( + "Barrier Test Guideline", + "https://example.com", + Arrays.asList(new IOTestGuidelineCheck("Check", RequirementLevel.MUST))); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + io.write(baos, guideline); + + Path jarPath = + buildTestJar(tempDir.toPath().resolve("barrier-test.jar"), baos.toByteArray(), 5); + + try (URLClassLoader classLoader = + new URLClassLoader(new java.net.URL[] {jarPath.toUri().toURL()}, null)) { + + URL folderUrl = classLoader.getResource("test-guidelines"); + assertNotNull(folderUrl, "Directory entry must exist in the JAR"); + assertTrue( + "jar".equals(folderUrl.getProtocol()), + "Expected jar: protocol, got: " + folderUrl.getProtocol()); + + CyclicBarrier barrier = new CyclicBarrier(2); + AtomicReference threadAError = new AtomicReference<>(); + + Thread threadA = + new Thread( + () -> { + try { + JarURLConnection conn = + (JarURLConnection) folderUrl.openConnection(); + // useCaches defaults to true — gets shared cached JarFile + JarFile jarFile = conn.getJarFile(); + Enumeration entries = jarFile.entries(); + assertTrue(entries.hasMoreElements()); + entries.nextElement(); // first entry OK + + barrier.await(); // signal: I am mid-iteration + barrier.await(); // wait: JarFile is now closed + + entries.nextElement(); // must throw + } catch (Throwable t) { + threadAError.set(t); + } + }, + "ThreadA-iterating"); + + Thread threadB = + new Thread( + () -> { + try { + barrier.await(); // wait: Thread A is mid-iteration + JarURLConnection conn = + (JarURLConnection) folderUrl.openConnection(); + // useCaches=true → same JarFile instance as Thread A + JarFile sharedJarFile = conn.getJarFile(); + sharedJarFile.close(); // closes the shared instance + barrier.await(); // signal: done + } catch (Throwable t) { + threadAError.compareAndSet(null, t); + } + }, + "ThreadB-closer"); + + threadA.start(); + threadB.start(); + threadA.join(10_000); + threadB.join(10_000); + + Throwable caught = threadAError.get(); + assertNotNull(caught, "Thread A must throw after the shared JarFile is closed"); + boolean isExpectedType = + (caught instanceof IllegalStateException + && caught.getMessage() != null + && caught.getMessage().contains("zip file closed")) + || (caught instanceof java.io.FileNotFoundException); + assertTrue( + isExpectedType, + "Expected IllegalStateException('zip file closed') [JDK ≤17] or " + + "FileNotFoundException [JDK 21] but got: " + + caught.getClass().getName() + + ": " + + caught.getMessage()); + } + } + + // ------------------------------------------------------------------------- + // TEST 2 — Verifies the fix: concurrent calls to readGuidelines must all + // return the correct number of guidelines with no errors and no data loss. + // + // The fix (listXmlFiles no longer closes the cached JarFile) means all + // threads share the JVM-cached instance and nobody closes it from under + // anyone else. The JVM owns the lifecycle. + // ------------------------------------------------------------------------- + @Test + void testConcurrentReadGuidelinesIsCorrectAfterFix(@TempDir File tempDir) throws Exception { + + GuidelineIO io = new GuidelineIO(TestAnalyzedProperty.class); + Guideline guideline = + new Guideline( + "Concurrency Fix Test Guideline", + "https://example.com", + Arrays.asList( + new IOTestGuidelineCheck("Check1", RequirementLevel.MUST), + new IOTestGuidelineCheck("Check2", RequirementLevel.SHOULD))); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + io.write(baos, guideline); + + Path jarPath = + buildTestJar( + tempDir.toPath().resolve("concurrent-test.jar"), + baos.toByteArray(), + GUIDELINE_FILE_COUNT); + + try (URLClassLoader classLoader = + new URLClassLoader(new URL[] {jarPath.toUri().toURL()}, null)) { + + int threadCount = 50; + int iterations = 200; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startGate = new CountDownLatch(1); + + List errors = Collections.synchronizedList(new ArrayList<>()); + List resultCounts = Collections.synchronizedList(new ArrayList<>()); + List> futures = new ArrayList<>(); + + for (int i = 0; i < iterations; i++) { + futures.add( + executor.submit( + () -> { + try { + startGate.await(); + List result = + io.readGuidelines(classLoader, "test-guidelines"); + resultCounts.add(result.size()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (Exception e) { + errors.add(e); + } + })); + } + + startGate.countDown(); + for (Future future : futures) { + future.get(30, TimeUnit.SECONDS); + } + executor.shutdown(); + + assertEquals( + 0, + errors.size(), + "No exceptions expected after fix, but got: " + errors.size()); + + long wrongCount = resultCounts.stream().filter(c -> c != GUIDELINE_FILE_COUNT).count(); + assertEquals( + 0, + wrongCount, + String.format( + "All %d calls must return exactly %d guidelines, but %d returned a" + + " different count", + iterations, GUIDELINE_FILE_COUNT, wrongCount)); + } + } +}