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

[Calvin Cheung]

Hi Steven,

I've only investigated the duplicate app class path issue and could 
reproduce it with JDK 21. I couldn't reproduce it with my mainline repo 
which includes fixes in the upcoming JDK 22.

I think the following fix in JDK 22 addresses the duplicate class path 
issue:
https://bugs.openjdk.org/browse/JDK-8304292

We can look into backporting the fix into JDK 21.

I'll let other folks to comment on the remaining issues.

thanks,
Calvin

On 12/8/23 9:56 AM, 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