Skip to content

Conversation

minborg
Copy link
Contributor

@minborg minborg commented Aug 11, 2025

Description

This PR proposes to update the ClassLoader implementation to properly guard access to the provided ByteBuffer when defining a class using defineClass(String, ByteBuffer, ...). Specifically, calls to SharedSecrets.getJavaNioAccess().acquireSession(ByteBuffer) and releaseSession(ByteBuffer) have been introduced to ensure safe and consistent buffer access throughout the native class definition process, even in the case of a ByteBuffer is backed by a MemorySegment.

Impact

This modification is internal to the ClassLoader implementation and does not affect the public API.
Improves the robustness and security of class loading from buffers.

Testing

Tier 1, 2, and 3 JDK tests pass on multiple platforms.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8365203: defineClass with direct buffer can cause use-after-free (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26724/head:pull/26724
$ git checkout pull/26724

Update a local copy of the PR:
$ git checkout pull/26724
$ git pull https://git.openjdk.org/jdk.git pull/26724/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26724

View PR using the GUI difftool:
$ git pr show -t 26724

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26724.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 11, 2025

👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Aug 11, 2025

@minborg This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8365203: defineClass with direct buffer can cause use-after-free

Reviewed-by: jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 34 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk
Copy link

openjdk bot commented Aug 11, 2025

@minborg The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

final class GuardByteBuffer {

@Test
void guardCrash() throws InterruptedException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to reproduce the crash using this test on a Mac. The original reproducer worked on a Windows machine.

Copy link
Member

@jaikiran jaikiran Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Per, I too couldn't reproduce the crash from the original reproducer (and this test) on macos and I found that a bit odd. I read up a bit about macos memory debugging tools and it turns out macos has a "Guard Malloc" implementation https://developer.apple.com/library/archive/documentation/Performance/Conceptual/ManagingMemory/Articles/MallocDebug.html which can be optionally enabled to debug issues like these. man libgmalloc has additional details about it.

So I ran the original reproducer again, this time with:

export DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib
java ... <that-reproducer>

and that consistently reproduces the crash on macos. I will build this PR locally and give it a try soon to make sure the crash no longer reproduces.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will build this PR locally and give it a try soon to make sure the crash no longer reproduces.

I tested this change locally with the reproducer and with libgmalloc. The crash no longer happens.

postDefineClass(c, protectionDomain);
return c;

SharedSecrets.getJavaNioAccess().acquireSession(b);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the fields in SharedSecrets are @Stable, we do not have to make a local copy in a static final field.

@minborg minborg marked this pull request as ready for review August 11, 2025 12:37
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 11, 2025
@mlbridge
Copy link

mlbridge bot commented Aug 11, 2025

Webrevs

SharedSecrets.getJavaNioAccess().acquireSession(b);
try {
Class<?> c = defineClass2(this, name, b, b.position(), len, protectionDomain, source);
postDefineClass(c, protectionDomain);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leave postDefineClass out of this acquire-release scope? I don't see any reason including this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters here because something looking to close the arena around the time that it wants to defineClass with memory allocated from that arena is broken.

@AlanBateman
Copy link
Contributor

The change looks okay. One thing to check if whether we have tests for JNI GetDirectBufferAddress when the byte buffer is a view of a memory segment.

@jaikiran
Copy link
Member

I think for the new test introduced, I don't think we should test for a crash. Instead maybe we can simplify it as follows to assert that a closed Arena doesn't allow a Class to be defined from the ByteBuffer. I think that will simplify the test and also make it more deterministic. This is what I had in mind:

/*
 * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug 8365203
 * @summary Tests guarding of ByteBuffers in ClassLoader::defineClass
 * @run junit GuardByteBuffer
 */

import java.lang.foreign.Arena;
import java.util.HexFormat;

import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertThrows;

final class GuardByteBuffer {
    @Test
    void guardCrash() {
        final byte[] classBytes = getClassBytes(); // get bytes of a valid class
        final var cl = new ClassLoader() {
            void tryCrash() {
                var arena = Arena.ofConfined();
                var byteBuffer = arena.allocate(classBytes.length).asByteBuffer();
                // Close the arena underneath
                arena.close();
                // expected to always fail because the arena
                // from which the ByteBuffer was constructed
                // has been closed
                assertThrows(IllegalStateException.class,
                        () -> defineClass(null, byteBuffer, null));
            }
        };
        for (int i = 0; i < 10000; i++) {
            cl.tryCrash();
        }
    }

    private static byte[] getClassBytes() {
        // unused. this is here just for reference
        final String source = """
                    public class NoOp {}
                """;
        // (externally) compiled content of the above "source", represented as hex
        final String classBytesHex = """
                cafebabe00000044000d0a000200030700040c000500060100106a6176612f
                6c616e672f4f626a6563740100063c696e69743e0100032829560700080100
                044e6f4f70010004436f646501000f4c696e654e756d6265725461626c6501
                000a536f7572636546696c650100094e6f4f702e6a61766100210007000200
                0000000001000100050006000100090000001d00010001000000052ab70001
                b100000001000a000000060001000000010001000b00000002000c
                """;

        return HexFormat.of().parseHex(classBytesHex.replaceAll("\n", ""));
    }

}

Copy link
Member

@jaikiran jaikiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Per for the update. This looks good to me.

It would be good to run this change and the test in our CI once to be sure nothing unexpected fails.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 27, 2025
@minborg
Copy link
Contributor Author

minborg commented Aug 27, 2025

It would be good to run this change and the test in our CI once to be sure nothing unexpected fails.

Thanks for the heads up on CI. It passes tier1-3. Thanks for the reviews!

@minborg
Copy link
Contributor Author

minborg commented Aug 27, 2025

/integrate

@openjdk
Copy link

openjdk bot commented Aug 27, 2025

Going to push as commit 19f0755.
Since your change was applied there have been 37 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 27, 2025
@openjdk openjdk bot closed this Aug 27, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 27, 2025
@openjdk
Copy link

openjdk bot commented Aug 27, 2025

@minborg Pushed as commit 19f0755.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs [email protected] integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants