8361300: Document exceptions for Unsafe offset methods #25945

liach · 2025-06-24T00:04:54Z

Unsafe throws IAE for misusing static vs instance fields, and it's revealed that AtomicXxxFieldUpdaters are using this mechanism to reject static fields. This is not a good practice, but we can at least document this so we don't accidentally introduce problems.

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-8361300: Document exceptions for Unsafe offset methods (Enhancement - P4)

Reviewers

John R Rose (@rose00 - Reviewer)
Volkan Yazici (@vy - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25945

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-24T00:05:23Z

👋 Welcome back liach! 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 · 2025-06-24T00:05:42Z

@liach 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:

8361300: Document exceptions for Unsafe offset methods

Reviewed-by: jrose, vyazici

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 69 new commits pushed to the master branch:

f95af74: 8364312: debug agent should set FD_CLOEXEC flag rather than explicitly closing every open file
72d1066: 8364722: Parallel: Move CLDG mark clearing to the end of full GC
0ceb366: 8356645: Javac should utilize new ZIP file system read-only access mode
... and 66 more: https://git.openjdk.org/jdk/compare/c4fbfa21030c9a0e8a3e0eed1b0a0988eba08ddb...master

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 · 2025-06-24T00:06:03Z

@liach 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.

ExE-Boss · 2025-06-24T17:43:30Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

+     * @throws NullPointerException if the field is {@code null}
+     * @throws IllegalArgumentException if the field is static


Maybe these checks could be done in Java like:

// implicit null check: if ((f.getModifiers() & ACC_STATIC) != 0) { throw new IllegalArgumentException("Field is static"); }

and for the staticField(Offset/Base) methods:

// implicit null check: if ((f.getModifiers() & ACC_STATIC) == 0) { throw new IllegalArgumentException("Field is not static"); }

Don't think anyone is willing to change code here...

That said, the only places that depend on this IAE behavior is the AtomicXxxFieldUpdater classes. All other sites pass trusted fields into these methods, of course besides the sun.misc.Unsafe users which definitely need this IAE. Still I personally prefer adding these checks to atomic field updaters instead of here to reduce risky dependencies.

As I commented on the C++ change, I think it's better to put the check in Java rather than C++.

The behavioral results are equivalent, right? It is fine to upgrade the InternalError to IAE; InternalError is by definition a poorly-defined state, and IAE is a less poorly defined way to report it. Changing that path to throw IAE is better, since it aligns with other field-query APIs. Because we control use of Unsafe (in this package) we can make small changes like this and just adjust our own code if something breaks.

Error checks and validations in Java code will be 10x clearer (more maintainable, scrutable to JITs) than logic in the C++ code.

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

src/hotspot/share/prims/unsafe.cpp

ExE-Boss · 2025-06-26T05:42:24Z

I noticed that the native implementation of Unsafe::array⁠(BaseOffset/IndexScale) refers to the non‑existent class java.lang.InvalidClassException instead of java.lang.IllegalArgumentException (which results in java.lang.NoClassDefFoundError being thrown):

jdk/src/hotspot/share/prims/unsafe.cpp

Lines 587 to 594 in 1ca008f

    
           static void getBaseAndScale(int& base, int& scale, jclass clazz, TRAPS) { 
        
             assert(clazz != nullptr, "clazz must not be null"); 
        
             oop mirror = JNIHandles::resolve_non_null(clazz); 
        
             Klass* k = java_lang_Class::as_Klass(mirror); 
        
             if (k == nullptr || !k->is_array_klass()) { 
        
               THROW(vmSymbols::java_lang_InvalidClassException());

liach · 2025-07-02T19:13:30Z

For arrayBaseOffset/IndexScale, I don't think I will change it here: they are methods that are exposed via sun.misc.Unsafe.

mlbridge · 2025-07-02T19:26:17Z

Webrevs

AlanBateman · 2025-07-03T05:43:21Z

src/java.base/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java

                          ? caller : tclass;
            this.tclass = tclass;
-            this.offset = U.objectFieldOffset(field);
+            this.offset = U.objectFieldOffset(field); // throws IAE for static


AtomicIntegerFieldUpdaterImpl already checks the field type and it is volatile. I think it would be better to expand these checks so that static volatile fields are rejected there rather than "accidentally" calling objectFieldOffset with a static field.

liach · 2025-07-03T14:04:33Z

Thanks for this permission to update the Atomic updaters. I found that there's no coverage in tck when these IAE on static fields are acknowledged recently, so I took the liberty to add them.

liach · 2025-07-03T18:37:59Z

I have added test cases to verify the current behaviors of Atomic field updaters and Unsafe memory address calculation methods, including untouched ones for arrays pointed out by @ExE-Boss. All of these are currently exposed through public APIs.

ExE-Boss · 2025-07-06T18:32:56Z

test/jdk/jdk/internal/misc/Unsafe/AddressComputationContractTest.java

+    void arrayBaseOffset() {
+        assertDoesNotThrow(() -> getUnsafe().arrayBaseOffset(int[].class));
+        assertThrows(NullPointerException.class, () -> getUnsafe().arrayBaseOffset(null));
+        // Caused by VM trying to throw java.lang.InvalidClassException (there's one in java.io instead)


Note that the one in java.io is also a checked exception (extends java.io.ObjectStreamException which extends java.io.IOException).

Yes, but there has been precedent of some sort of code throwing checked exceptions in a potentially-unchecked context, such as the lookup getter in proxy classes throw IllegalAccessException instead of IllegalAccessError.

Anyways, an java.io exception is definitely not suitable here. When sun.misc.Unsafe memory access methods are gone, we might simply change this to an IAE instead.

minborg · 2025-07-07T09:54:28Z

src/java.base/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java

            if (!Modifier.isVolatile(modifiers))
                throw new IllegalArgumentException("Must be volatile type");

+            if (Modifier.isStatic(modifiers))


Can we break out the Modifier tests in the three updater classes to a common method?

Should we include that in this patch?

Created https://bugs.openjdk.org/browse/JDK-8364544 to track that.

AlanBateman · 2025-07-07T09:56:07Z

src/java.base/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java


+            if (Modifier.isStatic(modifiers))
+                throw new IllegalArgumentException("Must not be a static field");
+


@viktorklang-ora @DougLea Should we update the Atomic*FieldUpdater to specify the IAE when the factory method is called with a static Field?

@AlanBateman Yeah, I think that makes sense—IIRC it's been that way in practice since forever, but making it clear in documentation seems like the right thing to do.

Tracked in 8360541; I think we can implement in a separate patch. Don't know if this is worth backporting to MRs.

Ah yes, I'd forgotten I created this issue to track clarifying the spec.

Sure, the clarification is long past being useful (since people should be using VarHandles) but why not.

liach · 2025-07-08T17:22:35Z

I think from our discussions, there is nothing else to change for this patch. Please correct me if my understanding is wrong, and if it isn't, I hope someone can hop in and leave a review.

rose00 · 2025-07-30T17:32:57Z

src/hotspot/share/prims/unsafe.cpp

  for (JavaFieldStream fs(k); !fs.done(); fs.next()) {
    Symbol *name = fs.name();
    if (name->equals(utf_name)) {
-      offset = fs.offset();


While you are here, if you keep this check, rename this particular function find_field_offset to find_nonstatic_field_offset (not the other one that takes the "must be static" flag).

The logic, as written, is difficult to understand because of the behavioral difference between the two overloading.

(I note that "instance field" is also a term of art in our code base, but "nonstatic_field" is more common.)

Alternatively, and probably better, take out this particular check (restoring equal access to static and non-static) and just rely on the checks in Java code. Such checks are almost always better (easier to reason about by humans and JITs) than checks in C++ code.

This particular method intentionally avoids accessing Java reflective objects to reduce startup overhead. I think a better approach may be returning error code and have Java code report a failure. Thoughts?

rose00 · 2025-07-30T17:55:51Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

-    private native long staticFieldOffset0(Field f);
-    private native Object staticFieldBase0(Field f);
+    private native long objectFieldOffset0(Field f); // throws IAE
+    private native long objectFieldOffset1(Class<?> c, String name); // throws InternalError


Ugh; this IE is part of the problem. You could push an IAE into the C++ code for symmetry, or start pushing more IAE logic into the Java code (my preference, but a bigger change perhaps).

I can tell you that the original intention of choosing InternalError is to mark places where the caller has a responsibility to make InternalError impossible. InternalError is not intended to trigger further processing; it denotes system failure.

rose00 · 2025-07-30T17:58:33Z

test/jdk/jdk/internal/misc/Unsafe/AddressComputationContractTest.java

+        assertDoesNotThrow(() -> getUnsafe().objectFieldOffset(AddressComputationContractTest.class, "instanceField"));
+        assertThrows(NullPointerException.class, () -> getUnsafe().objectFieldOffset(null, "instanceField"));
+        assertThrows(NullPointerException.class, () -> getUnsafe().objectFieldOffset(AddressComputationContractTest.class, null));
+        assertThrows(InternalError.class, () -> getUnsafe().objectFieldOffset(AddressComputationContractTest.class, "doesNotExist"));


This is a malformed test on a malformed API. VirtualMachineError exceptions signal system failure.

…fe-field-offset

rose00

This works for me.

liach · 2025-08-01T04:56:26Z

@vy Can you help review this patch too? For context, objectFieldOffset(Class, String) should only find trusted fields and is a bootstrap optimization, while the Field-accepting version is the generic API.

vy

I don't possess sufficient experience on Unsafe et al., though given there are no behavioral changes, I presume it should all be fine. I've verified the following:

Unsafe doc improvements
U::objectFieldOffset(Class,String) throws descriptive IE
U::objectFieldOffset1 is renamed to more descriptive knownObjectFieldOffset0
Atomic*FieldUpdater checks and their TCK counterparts (This could have actually been a separate PR, but I see that the component owners gave consent.)
AddressComputationContractTest

I guess you will have a follow-up JBS ticket (along with a PR? 😇) for @minborg's suggestion on consolidating checks in Atomic*FieldUpdater classes. For instance, I see AIFU.AIFUImpl::isAncestor is not even used.

vy · 2025-08-01T09:11:07Z

test/jdk/jdk/internal/misc/Unsafe/AddressComputationContractTest.java

+    }
+
+    @Test
+    void fastObjectFieldOffset() {


Nit: You may want to match the corresponding impl. method name, as you did in other test methods:

Suggested change

void fastObjectFieldOffset() {

void knownObjectFieldOffset() {

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

liach · 2025-08-08T17:16:20Z

Thanks for the reviews.

As a final note, I decide to keep the in C++ unsafe checks for now; the unsafe.cpp seems to indicate there are more contracts that I cannot verify right now, for things like offset conversions. So I will integrate this patch as-is.

/integrate

openjdk · 2025-08-08T17:17:26Z

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

241808e: 8364269: Simplify code cache API by storing adapter entry offsets in blob
1b3e231: 8360048: NMT crash in gtest/NMTGtests.java: fatal error: NMT corruption: Block at 0x0000017748307120: header canary broken
a26a6f3: 8364649: G1: Move collection set related full gc reset code into abandon_collection_set() method
... and 91 more: https://git.openjdk.org/jdk/compare/c4fbfa21030c9a0e8a3e0eed1b0a0988eba08ddb...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-08-08T17:17:48Z

@liach Pushed as commit cd50d78.

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

Document IAE for Unsafe offset methods

f5c1079

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 24, 2025

ExE-Boss reviewed Jun 24, 2025

View reviewed changes

Add static check

0c59f83

ExE-Boss reviewed Jun 24, 2025

View reviewed changes

src/hotspot/share/prims/unsafe.cpp Outdated Show resolved Hide resolved

Simplify

db465d7

liach changed the title ~~Document IAE for Unsafe offset methods~~ 8361300: Document exceptions for Unsafe offset methods Jul 2, 2025

liach marked this pull request as ready for review July 2, 2025 19:21

openjdk bot added the rfr Pull request is ready for review label Jul 2, 2025

AlanBateman reviewed Jul 3, 2025

View reviewed changes

liach added 2 commits July 3, 2025 08:52

Explicit static checks

904237c

Add tck coverage

c8fcf03

Test to verify observed internal unsafe behaviors

57f863b

ExE-Boss reviewed Jul 6, 2025

View reviewed changes

minborg reviewed Jul 7, 2025

View reviewed changes

AlanBateman reviewed Jul 7, 2025

View reviewed changes

rose00 suggested changes Jul 30, 2025

View reviewed changes

rose00 reviewed Jul 30, 2025

View reviewed changes

liach added 2 commits July 31, 2025 12:20

Merge branch 'master' of https://github.com/openjdk/jdk into doc/unsa…

e2e5f24

…fe-field-offset

Move exception to Java code, stress the field must be known

71584eb

Less specific reasons for IAE

d920ec1

rose00 approved these changes Jul 31, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 31, 2025

vy approved these changes Aug 1, 2025

View reviewed changes

Test update advised by volkan

1ce1415

openjdk bot removed the ready Pull request is ready to be integrated label Aug 1, 2025

ExE-Boss reviewed Aug 1, 2025

View reviewed changes

src/java.base/share/classes/jdk/internal/misc/Unsafe.java Show resolved Hide resolved

src/java.base/share/classes/jdk/internal/misc/Unsafe.java Show resolved Hide resolved

rose00 approved these changes Aug 1, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 1, 2025

vy approved these changes Aug 1, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 8, 2025

openjdk bot closed this Aug 8, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 8, 2025

graalvmbot mentioned this pull request Aug 8, 2025

[JDK-8364512] Adapt JDK-8361300: Document exceptions for Unsafe offset methods oracle/graal#11923

Merged

graalvmbot mentioned this pull request Aug 19, 2025

[GR-68735] Update labsjdk to 26+11-jvmci-b01 oracle/graal#11994

Merged

		* @throws NullPointerException if the field is {@code null}
		* @throws IllegalArgumentException if the field is static


		if (Modifier.isStatic(modifiers))
		throw new IllegalArgumentException("Must not be a static field");

	void fastObjectFieldOffset() {
	void knownObjectFieldOffset() {

8361300: Document exceptions for Unsafe offset methods #25945

8361300: Document exceptions for Unsafe offset methods #25945

Uh oh!

Conversation

liach commented Jun 24, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 24, 2025

Uh oh!

openjdk bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ExE-Boss commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach commented Jul 2, 2025

Uh oh!

mlbridge bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach commented Jul 3, 2025

Uh oh!

liach commented Jul 3, 2025

Uh oh!

ExE-Boss Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rose00 left a comment

Choose a reason for hiding this comment

Uh oh!

liach commented Aug 1, 2025

Uh oh!

liach commented Jun 24, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 24, 2025 •

edited

Loading

ExE-Boss commented Jun 26, 2025 •

edited

Loading

mlbridge bot commented Jul 2, 2025 •

edited

Loading

ExE-Boss Jul 6, 2025 •

edited

Loading