CDS can archive classpath entries more than once when a JAR manifest has Class-Path attributes

[Johan Sjolen]

Hi Steven,

The issue you saw in NMT is an indication that something is calling 
NMT's API in an unexpected manner. This is probably a symptom of the CDS 
bug, so when that bug is fixed NMT will no longer crash the JVM. There 
are plans to make NMT more resilient against these sorts of misuses, but 
those changes are planned for JDK-23.

Thank you for the in-depth research on this issue.

All the best,
Johan

On 2023-12-08 18:56, Steven Schlansker wrote:
> Hi hotspot-dev,
> 
> Recently, we started experiencing JVM crashes [1] and inexplicable
> IncompatibleClassChangeErrors in our testing environment. We use
> custom classloaders, NMT, and app-CDS.
> 
> #  Internal Error (virtualMemoryTracker.cpp:403), pid=20, tid=128
> #  Error: ShouldNotReachHere()
> #
> 
> # JRE version: OpenJDK Runtime Environment (Red_Hat-21.0.1.0.12-2)
> (21.0.1+12) (build 21.0.1+12-LTS)
> # Java VM: OpenJDK 64-Bit Server VM (Red_Hat-21.0.1.0.12-2)
> (21.0.1+12-LTS, mixed mode, sharing, tiered, compressed oops,
> compressed class ptrs, g1 gc, linux-amd64)
> # Problematic frame:
> # V  [libjvm.so+0x104a06c]
> VirtualMemoryTracker::add_reserved_region(unsigned char*, unsigned
> long, NativeCallStack const&, MEMFLAGS)+0x6fc
> 
> and,
> 
> java.lang.IncompatibleClassChangeError:
> com.paywholesail.components.util.ByteBuffers and
> com.paywholesail.components.util.ByteBuffers$ByteBufferPuttable
> disagree on InnerClasses attribute
> (I checked with javap, and it looks the same to me...)
> 
> At least for the ShouldNotReachHere, it looked like a definite JVM
> bug, so I have been trying to create a reproducing test case to make a
> good error report. I noticed that the crash only happens when NMT is
> combined with Class Data Sharing. At this point, I read the logs
> closely, and noticed:
> 
> [0.139s][warning][cds       ] shared class paths mismatch
> [0.151s][warning][cds,dynamic] Unable to use shared archive. The top
> archive failed to load: /.../prebake.jsa
> 
> So, I compared the expected and actual class path as printed by the
> JVM. In both cases, we run with `-cp lib/*` with a fixed set of
> library jars. Imagine my surprise when I find that the only difference
> is that the expected (archive-time) classpath includes
> lib/stax-ex-1.8.jar *twice*.
> 
> By running the generated shared archive file with `strings | grep`, I
> am able to verify that the `lib/stax-ex-1.8.jar` entry indeed is
> present in the archive twice.
> 
> I fixed up my JDK build environment and started sprinkling new logging
> and assertions through the archive creation code.
> 
> It looks like ClassLoader::add_to_app_classpath_entries can either
> check for duplicated classpath entries, or trust that the caller knows
> the element is new.
> This list of entries is built in part by
> ClassLoader::setup_app_search_path, which enumerates the classpath and
> adds entries one by one. In this case, duplicate checks are skipped,
> presumably because we trust the initial classpath not to have
> duplicates.
> 
> When an element is added in add_to_app_classpath_entries, for each
> jar, it calls process_jar_manifest. Among other things, this reads the
> MANIFEST.MF and looks for Class-Path entries, and loads those too.
> Indeed, our `jaxb-runtime` has such an entry for `stax-ex`. In this
> case, it does guard against duplicate entries.
> 
> I think there is a bug here: if a jar is added by a manifest's
> Class-Path from a jar *before* we finish processing the initial app
> class path, it can get added twice - first with a duplicate check via
> the manifest, and then a second time without checking for duplicates
> from the app classpath.
> 
> I believe this is reproducible on latest 21.0.1+12 with the following
> code and steps:
> 
> A.java:
> class A {
>      static {
>          System.err.println("A");
>      }
> }
> 
> class B {
>      public static void main(String[] args) {
>          System.err.println("hi!");
>          new A();
>      }
> }
> 
> MANIFEST.MF:
> Manifest-Version: 1.0
> Class-Path: B.jar
> 
> 
> % mkdir lib
> % javac A.java
> % javac B.java
> % jar -m META-INF/MANIFEST.MF -c -f lib/A.jar A.class
> % jar cf lib/B.jar B.class
> 
> % java -cp lib/B.jar:lib/A.jar -XX:ArchiveClassesAtExit=shared.jsa
> -XX:NativeMemoryTracking=summary B
> % strings shared.jsa| grep lib/
> lib/B.jar
> lib/A.jar
> 
> % java -cp lib/A.jar:lib/B.jar -XX:ArchiveClassesAtExit=shared2.jsa
> -XX:NativeMemoryTracking=summary B
> % strings shared2.jsa| grep lib/
> lib/A.jar
> lib/B.jar
> lib/B.jar
> 
> When A.jar is loaded first, the Class-Path manifest entry adds B.jar.
> Then, B.jar is added *again*, unconditionally.
> When B.jar is loaded first, the app classpath entry is created first.
> Then, the manifest entry is checked and since it is a duplicate, only
> one entry is added.
> 
> At this point I felt like I collected enough information to ask for
> some expert advice.
> Am I on the right track here, that this could be a bug resulting in
> duplicate classpath entries in the archive classpath, if a dependent
> jar comes in via a manifest class-path entry before the app classpath
> finishes processing? Could that possibly be the source of our
> assertion failures and IncompatibleClassChangeErrors?
> 
> As a related question, this makes me worry that using `-cp lib/*`
> might implicitly embed the filesystem enumeration order in the
> archive. Maybe the classpath order is not important when verifying,
> but at the very least, the wildcard enumeration order influences the
> build in a way I did not expect.
> 
> If my analysis sounds plausible, I can submit it via the Java bug system.
> 
> Thank you for any consideration and advice. Best,
> Steven
> 
> [1] https://gist.github.com/stevenschlansker/12d1eaeb363ae135c88a965048353b0e