8344168: Change Unsafe base offset from int to long #22095

wenshao · 2024-11-14T05:32:34Z

The type of the Unsafe base offset constant is int, which may cause overflow when adding int offsets, such as 8343925 (PR #22012). 8343984 (PR #22027) fixes most of the offset overflows in JDK, but ArraysSupport and CRC32C are still unfixed.

@liach proposed the idea of changing the Unsafe base offset to long, which is a complete solution to the Unsafe offset overflow. After discussing with @liach, I submitted this PR to implement @liach's idea.

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-8344168: Change Unsafe base offset from int to long (Enhancement - P4)

Reviewers

Chen Liang (@liach - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22095

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-14T05:33:25Z

👋 Welcome back swen! 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 · 2024-11-14T05:33:31Z

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

8344168: Change Unsafe base offset from int to long

Reviewed-by: liach

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

3f8a875: 8348880: Replace ConcurrentMap with AtomicReferenceArray for ZoneOffset.QUARTER_CACHE
fac63d4: 8348668: Prevent first resource cleanup in confined arena from escaping
2efb6aa: 8345314: Add a red–black tree as a utility data structure
a937f6d: 8343767: Enumerate StubGen blobs, stubs and entries and generate code from declarations
1858dc1: 8336382: Fix error reporting in loading AWT
22069ff: 8348975: Broken links in the JDK 24 JavaDoc API documentation, build 33
f81772a: 8348647: CDS dumping commits 3GB when large pages are used
fb0f2d2: 8300708: Some nsk jvmti tests fail with virtual thread wrapper due to jvmti missing some virtual thread support
bb528d5: 8348567: [ASAN] Memory access partially overflows by NativeCallStack
e0c2cb4: 8348663: [AIX] clang pollutes the burned-in library search paths of the generated executables
... and 3 more: https://git.openjdk.org/jdk/compare/cbe9ec530fc248be74766ff6ff32761cd415a6f0...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 · 2024-11-14T05:34:05Z

@wenshao The following labels will be automatically applied to this pull request:

core-libs
hotspot
nio

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

wenshao · 2024-11-14T05:47:11Z

/label remove hotspot nio

mlbridge · 2024-11-14T05:47:12Z

Webrevs

openjdk · 2024-11-14T05:47:53Z

@wenshao
The hotspot label was successfully removed.

The nio label was successfully removed.

dholmes-ora

Looking at this I'm not at all convinced this is the right thing to do. The ARRAY_BYTE_BASE_OFFSET is a small value - it is an int.

I understand there is concern about integer arithmetic overflow, but I'm not convinced this is where it needs to be addressed.

dholmes-ora · 2024-11-14T08:08:08Z

src/java.base/share/classes/java/util/zip/CRC32C.java

-            int alignLength
+            long alignLength
                    = (8 - ((Unsafe.ARRAY_BYTE_BASE_OFFSET + off) & 0x7)) & 0x7;
-            for (int alignEnd = off + alignLength; off < alignEnd; off++) {
+            for (long alignEnd = off + alignLength; off < alignEnd; off++) {


I think casting the (now) long expression back to int makes more sense here.

alignLength is a very small value, which is an int, but alignEnd needs to be a long. Here, changing alignLength to a long can avoid alignEnd overflow.

liach · 2024-11-14T13:44:10Z

I think you should convert all array index scales to long too. They are susceptible to the same overflow problem (actually more susceptible as they involve in integer multiplications)

Glavo · 2024-11-14T14:41:13Z

I'm concerned that such a change could have unintended consequences, and perhaps deprecating them and providing new long constants would be a better option.

liach · 2024-11-14T14:57:45Z

@Glavo This change only happens to jdk.internal.misc.Unsafe. The values in sun.misc.Unsafe are untouched, and they are already deprecated for removal.

wenshao · 2024-11-14T15:06:20Z

I think you should convert all array index scales to long too. They are susceptible to the same overflow problem (actually more susceptible as they involve in integer multiplications)

The only places in JDK where there is a risk of offset overflow using Unsafe index scale are jdk.incubator.vector.XXXVector, and they are all explicitly converted to long type. Can we not change it yet?

liach · 2024-11-14T15:48:27Z

I am currently busy with a few things in ClassFile API. I can soon come back onto this and might send PRs to you to migrate risky use of array index scales too.

src/java.base/share/classes/jdk/internal/foreign/Utils.java

dean-long · 2024-11-14T23:20:23Z

Doesn't this break any apps that use these offsets? Aren't these fields part of the public API of Unsafe, so changing them requires a CSR?

ExE-Boss · 2024-11-15T01:05:10Z

@dean-long

Doesn't this break any apps that use these offsets? Aren't these fields part of the public API of Unsafe, so changing them requires a CSR?

This is the encapsulated JDK‑internal Unsafe in java.base, not the legacy sun.misc.Unsafe.

dean-long

@ExE-Boss , are you saying the CSR doesn't apply to jdk.internal.*? The CSR FAQ says:
"Changes to public and exported APIs in jdk.*packages."
Whether or not it needs a CSR, I think there are apps outside the JDK that will break because of this change.

AlanBateman · 2024-11-16T08:40:12Z

@ExE-Boss , are you saying the CSR doesn't apply to jdk.internal.*? The CSR FAQ says: "Changes to public and exported APIs in jdk.*packages." Whether or not it needs a CSR, I think there are apps outside the JDK that will break because of this change.

jdk.internal.** is JDK internal, none of these packages are exported. It's okay to make changes in any release, any builds. Nothing outside of the JDK should be depending on internal APIs like this.

wenshao · 2024-11-22T00:45:00Z

It's been a week, can anyone help me complete this PR?

dean-long · 2024-11-22T04:23:26Z

@ExE-Boss , are you saying the CSR doesn't apply to jdk.internal.*? The CSR FAQ says: "Changes to public and exported APIs in jdk.*packages." Whether or not it needs a CSR, I think there are apps outside the JDK that will break because of this change.

jdk.internal.** is JDK internal, none of these packages are exported. It's okay to make changes in any release, any builds. Nothing outside of the JDK should be depending on internal APIs like this.

Netty, for example, has code to access jdk.internal.misc.Unsafe through reflection.

Should the CSR FAQ be updated to remove references to jdk.*, or are there some jdk.* packages that require a CSR? As far as I can tell, none are exported.

If later on someone decides to change these fields again, maybe to something like short, char, or byte, that better represents the possible value range, then we get the overflow problem again. How do we guard against that? Do we have regression tests for the overflow problem?

There are other idioms we could use if casting with (long) is considered ugly. For example:

long ARRAY_BYTE_BASE_OFFSET = Unsafe.ARRAY_BYTE_BASE_OFFSET;
long offset = ARRAY_BYTE_BASE_OFFSET + index;

long offset = Unsafe.ARRAY_BYTE_BASE_OFFSET;
offset += index;

long offset = Long.sum(Unsafe.ARRAY_BYTE_BASE_OFFSET, index);

wenshao · 2024-11-22T07:10:59Z

Explicit conversion to long is not ugly, but it is easy to be overlooked and cause bugs. Experienced JDK Committers can also make mistakes. PR #22027 fixes many problems, which proves this. The improvement of this PR is to avoid this type of mistake.

ExE-Boss · 2024-11-22T10:27:17Z

Should the CSR FAQ be updated to remove references to jdk.*, or are there some jdk.* packages that require a CSR? As far as I can tell, none are exported.

The jdk.incubator.* packages (such as jdk.incubator.vector) are exported.

AlanBateman · 2024-11-22T10:37:36Z

Should the CSR FAQ be updated to remove references to jdk., or are there some jdk. packages that require a CSR? As far as I can tell, none are exported.

The "Design principles" section of JEP 200 may be help. Standard modules, as in java.* modules, only export standard API packages. Non-standard may export non-standard APIs. The JDK has several jdk.* modules that export non-standard APIs in the com.sun.* and jdk.* name spaces.

…t_202411

wenshao · 2024-12-05T23:10:59Z

JDK 25 has been launched, please help to continue reviewing this PR

bridgekeeper · 2025-01-03T00:22:06Z

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

liach · 2025-01-03T17:04:28Z

Bringing this to attention again; by using the long type, this patch avoids potentially erroneous integer arithmetic used in memory address calculations.

…t_202411

liach

Tier 1-3 passes on Oracle's CI.

wenshao · 2025-01-30T15:55:58Z

/integrate

openjdk · 2025-01-30T15:57:17Z

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

5d5b294: 8349070: Fix riscv and ppc build errors caused by JDK-8343767
3f8a875: 8348880: Replace ConcurrentMap with AtomicReferenceArray for ZoneOffset.QUARTER_CACHE
fac63d4: 8348668: Prevent first resource cleanup in confined arena from escaping
2efb6aa: 8345314: Add a red–black tree as a utility data structure
a937f6d: 8343767: Enumerate StubGen blobs, stubs and entries and generate code from declarations
1858dc1: 8336382: Fix error reporting in loading AWT
22069ff: 8348975: Broken links in the JDK 24 JavaDoc API documentation, build 33
f81772a: 8348647: CDS dumping commits 3GB when large pages are used
fb0f2d2: 8300708: Some nsk jvmti tests fail with virtual thread wrapper due to jvmti missing some virtual thread support
bb528d5: 8348567: [ASAN] Memory access partially overflows by NativeCallStack
... and 4 more: https://git.openjdk.org/jdk/compare/cbe9ec530fc248be74766ff6ff32761cd415a6f0...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-30T15:57:37Z

@wenshao Pushed as commit fdfb68c.

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

dougxc · 2025-02-10T17:08:34Z

I'm curious why arrayOopDesc::base_offset_in_bytes was not included in this change - does it not face the same overflow problem?

liach · 2025-02-10T18:54:26Z

Re dougxc: This migration is specific to the Java language. I am not so sure about the C++ counterparts, especially that C++ has unsigned types that can complicate things. Another motivation of the Java change is that unsafe is widely used in the Java codebase so upgrading the type can potentially make future usages safer. For example, this already revealed a misuse of the array base offset in the benchmarks in #23393.

dougxc · 2025-02-10T21:39:00Z

Thanks for the clarification @liach . This means we only need to focus on the Java code in https://bugs.openjdk.org/browse/JDK-8349743 (cc @mur47x111).

long Unsafe offset

c0c283e

openjdk bot added nio nio-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 14, 2024

wenshao changed the title ~~Change Unsafe base offset from int to long~~ 8344168: Change Unsafe base offset from int to long Nov 14, 2024

openjdk bot added the rfr Pull request is ready for review label Nov 14, 2024

openjdk bot removed hotspot hotspot-dev@openjdk.org nio nio-dev@openjdk.org labels Nov 14, 2024

dholmes-ora reviewed Nov 14, 2024

View reviewed changes

JornVernee reviewed Nov 14, 2024

View reviewed changes

src/java.base/share/classes/jdk/internal/foreign/Utils.java Outdated Show resolved Hide resolved

long BaseAndScale base, from @JornVernee

f33e9d3

dean-long suggested changes Nov 15, 2024

View reviewed changes

wenshao and others added 4 commits November 27, 2024 01:42

Merge remote-tracking branch 'upstream/master' into long_unsafe_offse…

cb8487d

…t_202411

Make base offset method and invalid offset long

03ba260

Fix type expectations

6bf7e8a

Typos

6d4bb36

wenshao added 4 commits January 26, 2025 00:04

Merge remote-tracking branch 'upstream/master' into long_unsafe_offse…

e758781

…t_202411

fix DecimalDigits

76f6f3f

Merge remote-tracking branch 'upstream/master' into long_unsafe_offse…

d1d5bdf

…t_202411

copyright

3e16ea0

liach approved these changes Jan 30, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 30, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 30, 2025

openjdk bot closed this Jan 30, 2025

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

graalvmbot mentioned this pull request Jan 30, 2025

[JDK-8345672] Adapt JDK-8344168: Change Unsafe base offset from int to long oracle/graal#10602

Merged

wenshao mentioned this pull request Jan 31, 2025

8349142: [JMH] compiler.MergeLoadBench.getCharBV fails #23392

Closed

3 tasks

graalvmbot mentioned this pull request Feb 12, 2025

[GR-61993] Update labsjdk to 25+9-jvmci-b01 oracle/graal#10672

Merged

8344168: Change Unsafe base offset from int to long #22095

8344168: Change Unsafe base offset from int to long #22095

Uh oh!

Conversation

wenshao commented Nov 14, 2024 • 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 Nov 14, 2024

Uh oh!

openjdk bot commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 14, 2024

Uh oh!

wenshao commented Nov 14, 2024

Uh oh!

mlbridge bot commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Nov 14, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

wenshao Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

liach commented Nov 14, 2024

Uh oh!

Glavo commented Nov 14, 2024

Uh oh!

liach commented Nov 14, 2024

Uh oh!

wenshao commented Nov 14, 2024

Uh oh!

liach commented Nov 14, 2024

Uh oh!

Uh oh!

dean-long commented Nov 14, 2024

Uh oh!

ExE-Boss commented Nov 15, 2024

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Nov 16, 2024

Uh oh!

wenshao commented Nov 22, 2024

Uh oh!

dean-long commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenshao commented Nov 22, 2024

Uh oh!

ExE-Boss commented Nov 22, 2024

Uh oh!

AlanBateman commented Nov 22, 2024

Uh oh!

wenshao commented Dec 5, 2024

Uh oh!

bridgekeeper bot commented Jan 3, 2025

Uh oh!

liach commented Jan 3, 2025

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jan 30, 2025

Uh oh!

openjdk bot commented Jan 30, 2025

Uh oh!

openjdk bot commented Jan 30, 2025

wenshao commented Nov 14, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 14, 2024 •

edited

Loading

mlbridge bot commented Nov 14, 2024 •

edited

Loading

dean-long commented Nov 22, 2024 •

edited

Loading