On Tue, 30 Sep 2025 17:37:31 GMT, Brent Christian <[email protected]> wrote:
>> ### Background
>>
>> - ClassLoader.defineClass can receive class data in the form of arrays or
>> ByteBuffers.
>> - For array-backed data (defineClass1), a defensive copy is made before
>> passing it to JVM_DefineClassWithSource().
>> - For Direct-ByteBuffer variants (defineClass2), no defensive copy is made,
>> which creates a risk that the underlying bytes could be modified while the
>> JVM is processing them.
>> - Although a caller could always modify a buffer before a defensive copy is
>> made — a race condition that cannot be completely prevented — the **_main
>> concern_** is ensuring that the JVM never processes class bytes that are
>> being concurrently modified.
>>
>> ### Problem
>>
>> - Concurrent modification risk during processing: while we cannot prevent
>> pre-copy modifications, we **_must prevent the JVM from using class bytes
>> that are being modified concurrently._**
>> - Performance concerns: defensive copies have a cost, especially for direct
>> byte buffers. Making copies unnecessarily for trusted class loaders (like
>> the built-in class loader) would hurt performance.
>>
>> ### Solution
>>
>> - Make a defensive copy of the direct byte-buffer only when the class loader
>> is **NOT** a built-in/trusted class loader.
>> - For the built-in class loader, skip the copy because the JVM can guarantee
>> that the buffer contents remain intact.
>>
>> This approach ensures the integrity of class bytes processes for untrusted
>> or custom class loaders while minimizing performance impact for trusted or
>> built-in loaders.
>>
>> ### Benchmark
>>
>> A JMH benchmark has been added to measure the potential cost of the
>> defensive copy. The results indicate that the performance impact is minimal
>> and largely insignificant.
>>
>> **Before:**
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> ClassLoaderDefineClass.testDefineClassByteBufferDirect avgt 15 8387.247
>> ± 1405.681 ns/op
>> ClassLoaderDefineClass.testDefineClassByteBufferHeap avgt 15 8971.739
>> ± 1020.288 ns/op
>> Finished running test
>> 'micro:org.openjdk.bench.java.lang.ClassLoaderDefineClass'
>> Test report is stored in
>> /Users/xuemingshen/jdk26/build/macosx-aarch64/test-results/micro_org_openjdk_bench_java_lang_ClassLoaderDefineClass
>>
>>
>> **After:**
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> ClassLoaderDefineClass.testDefineClassByteBufferDirect avgt 15 8521.881
>> ± 2002.011 ns/op
>> ClassLoaderDefineClass.testDefineClassByt...
>
> src/java.base/share/classes/java/lang/ClassLoader.java line 1075:
>
>> 1073: }
>> 1074:
>> 1075: private Class<?> defineClass(String name, ByteBuffer b, int len,
>> ProtectionDomain pb) {
>
> Is an additional method really needed?
> Couldn't we just add a new local `ByteBuffer` reference, point it to either
> `b` (if trusted) or the newly allocated BB if not, and continue as before,
> passing the new reference to `defineClass2()`?
The separate method keeps it easier to audit (and review) so I'd prefer to keep
it as proposed. It is very possible that we will have additional cases to trust
in the future and it would complicated the conditions in the caller if
everything is in one method.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27569#discussion_r2392386298
