8366495: Incorrect minimum memory size allocated in allocateNativeInternal()

[offamitkumar]

Originally Reported in OpenJ9, fix by @AditiS11 present here: ibmruntimes/openj9-openjdk-jdk25#32

These test failure were reported in OpenJ9 (x86), I can't reproduce on my system (s390x):

java/foreign/TestFill.java
java/foreign/TestSegments.java
java/foreign/TestSegmentBulkOperationsContentHash.java
java/foreign/TestStringEncoding.java
java/foreign/TestVarArgs.java

        // Always allocate at least some memory so that zero-length segments have distinct
        // non-zero addresses.
        alignedSize = Math.max(1, alignedSize);

Here minimum-allocated size will be 1, which is incorrect because

    private static void initNativeMemory(long address, long byteSize) {
        for (long i = 0; i < byteSize; i += Long.BYTES) {
            UNSAFE.putLongUnaligned(null, address + i, 0);
        }
    }

initNativeMemory() is going to write Long.

---

Progress

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

Integration blocker

 ⚠️ Failed to retrieve information on issue 8366495. Please make sure it exists and is accessible.

Issue

• ⚠️ Failed to retrieve information on issue 8366495.

Reviewers

• Per Minborg (@minborg - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27027

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back amitkumar! 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]

@offamitkumar This change is no longer ready for integration - check the PR body for details.

[openjdk]

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

[mlbridge]

Webrevs

• 01: Full - Incremental (18c67d55)
• 00: Full (8bbef849)

[minborg]

Thanks for spotting this bug! It seems like a bug that I introduced, and so, thanks again!

[jaikiran]

Hello Amit, would it be possible to add a jtreg test for this? Do you know what was the code path or values for the allocateNativeSegment() call which triggered this issue. Having that replicated in a regression test I think would prevent this from happening again.

[offamitkumar]

Hello Amit, would it be possible to add a jtreg test for this? Do you know what was the code path or values for the allocateNativeSegment() call which triggered this issue. Having that replicated in a regression test I think would prevent this from happening again.

Hi @jaikiran ,

I will spend some time over it, current jtreg tests are unable to detect it. This is information Aditi gave me regarding the failure in OpenJ9: In openj9 the write is happening but it is corrupting the header bytes added by OMR. And the failure is thrown by OMR when it validates the memory segment.

[jaikiran]

I think it's OK (and understandable) if the jtreg test doesn't reproduce a JVM crash itself. What would be good, is to have the same API call(s) with those relevant values being invoked from the jtreg test. It may be that we already have a jtreg test which exercises those values and if we are able to identify such an existing test then it would be good to mention it here or in the JBS issue and we won't have to introduce a new one.

[jaikiran]

I had a deeper look at this and this specific issue can happen only when the byteSize is 0 for example when calling Arena.allocate(). The testZeroLengthNativeSegment() test method in test/jdk/java/foreign/TestSegments.java already covers this code path and exercises this bug. So I think it's fine if you don't add any additional test as part of this PR. I just have one more review comment which I'll add inline.

[jaikiran]

A few lines above this code there's a comment which says:

// Align the allocation size up to a multiple of 8 so we can init the memory with longs
long alignedSize = init ? Utils.alignUp(byteSize, Long.BYTES) : byteSize;

Should we have a similar comment as well as a check for init here? After all, we are increasing this size here because of init initializing the allocated memory. So perhaps something like:

// Always allocate at least some memory so that zero-length segments have distinct
// non-zero addresses. If we are initializing the allocated memory, then use a minimum
// size of 8 because we init the memory with longs.
alignedSize = Math.max((init ? Long.BYTES : 1), alignedSize);

If you do use this newer proposed change, then please have it verified against the original reproducer.

[offamitkumar]

The testZeroLengthNativeSegment() test method in test/jdk/java/foreign/TestSegments.java already covers this code path and exercises this bug.

Hi @jaikiran ,
I tried to spend sometime over it as current tests were passing, but couldn't reproduce the failure on openjdk. I guess, we will just pass it for now.

[jaikiran]

but couldn't reproduce the failure on openjdk.

Not being able to generate a JVM crash or a test failure from that test is understandable and OK. Such memory access issues aren't always reproducible in a deterministic manner. So yes, it's OK to not introduce a new test.

[minborg]

The new changes look good. Thanks for fixing this issue. Please update the title of the PR so we can integrate this one.

[minborg]

/reviewers 2

[openjdk]

@minborg
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).