8357466: Create test for Ciphers that are using ByteBuffers backed by MemorySegments #26967

mpdonova · 2025-08-27T18:09:55Z

This PR extends security tests to use ByteBuffers backed by MemorySegments. Tests in the areas of Signature, Cipher, MessageDigest, and Mac are updated.

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-8357466: Create test for Ciphers that are using ByteBuffers backed by MemorySegments (Enhancement - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26967

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

Using diff file

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

Using Webrev

Link to Webrev Comment

… MemorySegments

bridgekeeper · 2025-08-27T18:11:00Z

👋 Welcome back mdonovan! 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-08-27T18:12:19Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-08-27T18:13:21Z

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

security

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 · 2025-08-27T18:18:58Z

Webrevs

fandreuz · 2025-08-27T19:47:03Z

test/jdk/javax/crypto/CipherSpi/CipherByteBufferOverwriteTest.java

+
+            System.out.println("All Tests Passed");
+        } finally {
+            arena.close();


Would it make sense to initialize arena in a try-with-resources, so you can get rid of the explicit .close()?

I considered that for a while but I couldn't see a way to make it work without completely refactoring the whole test.

Wouldn't it work if you pass arena as a param to the prepareBuffers. This way you can call try-with-resources for each test and get rid of arena.close().

Also, it's only case MEMORY_SEGMENT: which needs arena at all, so you can just pass null to all others and only for test#3

// Test#3: against ByteBuffer backed by MemorySegment try (Arena arena = Arena.ofConfined()) { prepareBuffers(BufferType.MEMORY_SEGMENT, useRO, buf.length, buf, 0, PLAINTEXT_SIZE, offset, arena); runTest(offset, expectedPT, expectedCT); System.out.println("\tMEMSEGMENT: passed"); }

all others will look like this prepareBuffers(BufferType.ALLOCATE, useRO, buf.length, buf, 0, PLAINTEXT_SIZE, offset, null);, making it a minor refactor, as this method is only used here.

What do you think?

myankelev

Looks good, just one comment and a few nitpicks. IMO nits are not worth doing unless there is another commit.

myankelev · 2025-09-01T12:48:39Z

test/jdk/sun/security/pkcs11/Cipher/TestSymmCiphers.java

-        debugBuf.setLength(0);
+    }
+
+    private static void execTest(Cipher cipher, byte[]answer,


nit:

Suggested change

private static void execTest(Cipher cipher, byte[]answer,

private static void execTest(Cipher cipher, byte[] answer,

myankelev · 2025-09-01T12:50:43Z

test/jdk/javax/crypto/Cipher/GCMAPI.java

@@ -68,6 +69,13 @@ public static void main(String[] args) throws Exception {
        updateAADFail((ByteBuffer) null);
        updateAADPass(bb);

+        try(Arena arena = Arena.ofConfined()) {


nit:

Suggested change

try(Arena arena = Arena.ofConfined()) {

try (Arena arena = Arena.ofConfined()) {

The same for:

test/jdk/javax/crypto/Mac/ByteBuffers.java

test/jdk/javax/crypto/CipherSpi/DirectBBRemaining.java

test/jdk/javax/crypto/Cipher/ByteBuffers.java

test/jdk/java/security/MessageDigest/ByteBuffers.java

test/jdk/sun/security/pkcs11/Signature/ByteBuffers.java

test/jdk/sun/security/pkcs11/Cipher/TestPaddingOOB.java

test/jdk/sun/security/pkcs11/Cipher/TestSymmCiphers.java

test/jdk/sun/security/pkcs11/MessageDigest/ByteBuffers.java

myankelev · 2025-09-01T13:06:38Z

test/jdk/javax/crypto/CipherSpi/CipherByteBufferOverwriteTest.java

+
+            System.out.println("All Tests Passed");
+        } finally {
+            arena.close();


Wouldn't it work if you pass arena as a param to the prepareBuffers. This way you can call try-with-resources for each test and get rid of arena.close().

Also, it's only case MEMORY_SEGMENT: which needs arena at all, so you can just pass null to all others and only for test#3

// Test#3: against ByteBuffer backed by MemorySegment try (Arena arena = Arena.ofConfined()) { prepareBuffers(BufferType.MEMORY_SEGMENT, useRO, buf.length, buf, 0, PLAINTEXT_SIZE, offset, arena); runTest(offset, expectedPT, expectedCT); System.out.println("\tMEMSEGMENT: passed"); }

all others will look like this prepareBuffers(BufferType.ALLOCATE, useRO, buf.length, buf, 0, PLAINTEXT_SIZE, offset, null);, making it a minor refactor, as this method is only used here.

What do you think?

myankelev · 2025-09-01T13:08:14Z

test/jdk/javax/crypto/Cipher/GCMAPI.java

@@ -30,6 +30,7 @@

 import javax.crypto.*;
 import javax.crypto.spec.*;


Nit: Could you please remove the wildcard import here and from other files touched?

wangweij · 2025-09-04T14:05:47Z

Can we also add some negative tests? (This could be done in a different PR). For example, trying to use a read-only ByteBuffer for output, or trying to use a ByteBuffer after the arena has been closed.

mpdonova · 2025-09-04T17:00:39Z

Can we also add some negative tests? (This could be done in a different PR). For example, trying to use a read-only ByteBuffer for output, or trying to use a ByteBuffer after the arena has been closed.

Created JDK-8366912 for negative tests.

wangweij · 2025-09-05T13:33:58Z

There is a recent bug fix at #27081 when a slice of an array-based buffer is used. Can you investigate why it has not been caught by this test? Should it be enhanced? Thanks.

mpdonova · 2025-09-05T15:55:43Z

There is a recent bug fix at #27081 when a slice of an array-based buffer is used. Can you investigate why it has not been caught by this test? Should it be enhanced? Thanks.

There are a lot of tests under test/jdk/com/sun/crypto/provider that I didn't look at. There are some that explicitly test ByteBuffers (such as Cipher/AEAD/OverlapByteBuffer.java) and others that could possibly be extended to include them. I think updating all the tests would be unnecessary but I can update the tests that explicitly use ByteBuffer.

8357466: Create test for Ciphers that are using ByteBuffers backed by…

7c4c676

… MemorySegments

openjdk bot added security security-dev@openjdk.org rfr Pull request is ready for review labels Aug 27, 2025

fandreuz reviewed Aug 27, 2025

View reviewed changes

myankelev reviewed Sep 1, 2025

View reviewed changes

mpdonova added 2 commits September 2, 2025 08:12

Merge branch 'master' into cipher-ffm

e9ab73a

addressed PR comments and refactored to use try-with-resources

b0e61bb

added null-check for arena argument

16991ba

	private static void execTest(Cipher cipher, byte[]answer,
	private static void execTest(Cipher cipher, byte[] answer,

	try(Arena arena = Arena.ofConfined()) {
	try (Arena arena = Arena.ofConfined()) {

		@@ -30,6 +30,7 @@

		import javax.crypto.*;
		import javax.crypto.spec.*;

8357466: Create test for Ciphers that are using ByteBuffers backed by MemorySegments #26967

Are you sure you want to change the base?

8357466: Create test for Ciphers that are using ByteBuffers backed by MemorySegments #26967

Conversation

mpdonova commented Aug 27, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Aug 27, 2025

Uh oh!

openjdk bot commented Aug 27, 2025

Uh oh!

openjdk bot commented Aug 27, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

fandreuz Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

mpdonova Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev left a comment

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

myankelev Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

wangweij commented Sep 4, 2025

Uh oh!

mpdonova commented Sep 4, 2025

Uh oh!

wangweij commented Sep 5, 2025

Uh oh!

mpdonova commented Sep 5, 2025

Uh oh!

Uh oh!

mpdonova commented Aug 27, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 27, 2025 •

edited

Loading