8302111: Serialization considerations #3278

GoeLin · 2025-02-17T16:39:14Z

I backport this to match 17.0.13-oracle based on the commit to 21.

I had to resolve several files, two of them considerably:

src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java
Resolved larger chunk.
src/java.base/share/classes/com/sun/crypto/provider/DHPublicKey.java
Resolved some code.

The new code in DHPublic/PrivateKey depends on the removal
of IOExceptions in JDK-8297065 "DerOutputStream operations should not throw IOExceptions".
This change is in 21 but not in 17. Thus code backported from 21 calls functions of
DerOutputStream that do not throw an exception in 21, but do so in 17.
JDK-8297065 makes the point that these exceptions can never be thrown, which also
holds for 17. So I just catch and ignore them.

Also, I had to adapt code because an exception constructor with cause for
InvalidObjectException is missing in 17.
This was added by JDK-8282696 "Add constructors taking a cause
to InvalidObjectException and InvalidClassException" in 19.
I added a call to initCause() to store the causing exception.

I double-checked that JDK-8297065 was not backported
by Oracle. Backporting this change would simplify matters considerably.

The remaining files only needed trivial resolves:

src/java.base/share/classes/java/security/Permissions.java
src/java.base/share/classes/java/security/SignedObject.java
Only Copyright.
src/java.base/share/classes/java/security/Timestamp.java
Copyright and import.
src/java.base/share/classes/java/security/UnresolvedPermissionCollection.java
src/java.base/share/classes/java/security/cert/CertificateRevokedException.java
src/java.base/share/classes/sun/security/provider/DRBG.java
src/java.base/share/classes/sun/security/util/ObjectIdentifier.java
src/java.base/share/classes/sun/security/x509/AlgIdDSA.java
Only Copyright.
src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Context.java
Resolved imports.
src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5InitCredential.java
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecureRandom.java
Only Copyright.

I manually ran these tests on linux x86_64:

test/jdk/com/sun/security
test/jdk/java/awt/security
test/jdk/java/net/httpclient/security
test/jdk/java/net/httpclient/websocket/security
test/jdk/java/security
test/jdk/javax/management/security
test/jdk/javax/security
test/jdk/jdk/security
test/jdk/security
test/jdk/sun/security

Our well-known nightly tests passed, too.

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
JDK-8302111 needs maintainer approval

Issue

JDK-8302111: Serialization considerations (Bug - P4 - Approved)

Reviewers

Richard Reingruber (@reinrich - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/3278/head:pull/3278
$ git checkout pull/3278

Update a local copy of the PR:
$ git checkout pull/3278
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/3278/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3278

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/3278.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-17T16:39:28Z

👋 Welcome back goetz! 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-02-17T16:40:11Z

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

8302111: Serialization considerations

Reviewed-by: rrich

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

01d107a: 8339728: [Accessibility,Windows,JAWS] Bug in the getKeyChar method of the AccessBridge class
2846c9e: 8346828: javax/swing/JScrollBar/4865918/bug4865918.java still fails in CI
715d37e: 8346324: javax/swing/JScrollBar/4865918/bug4865918.java fails in CI
9f48d42: 8338595: Add more linesize for MIME decoder in macro bench test Base64Decode
f7a071e: 8337222: gc/TestDisableExplicitGC.java fails due to unexpected CodeCache GC
d8ae039: 8314975: JavadocTester should set source path if not specified
45a35e8: 8301989: new javax.swing.text.DefaultCaret().setBlinkRate(N) results in NPE
15730c0: 8274893: Update java.desktop classes to use try-with-resources
b2f814e: 8342098: Write a test to compare the images
ceea882: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java
... and 10 more: https://git.openjdk.org/jdk17u-dev/compare/2691c1428c592f952a0093e0cae3244ab48246c0...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-02-17T16:40:26Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2025-02-17T16:44:51Z

Webrevs

GoeLin · 2025-02-20T08:55:53Z

src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java

     * @param p the prime modulus
     * @param g the base generator
     *
     * @throws ProviderException if the key cannot be encoded


Hi @reinrich,
thanks for looking at this backport. I fixed DHPrivateKey accordingly.

reinrich · 2025-02-20T08:03:22Z

src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java

+            c = decode(encodedKeyIntern);
+        } catch (IOException e) {
+            InvalidObjectException ioe = new InvalidObjectException("Invalid encoding");
+            if (ioe != null) {


Isn't this condition always true?
Note also that the throw below will throw a NPE if it ioe was null.
I think you can actually remove the condition.

My inital desing was
throw new InvalidObjectException("Invalid encoding").initCause(e);
But the compiler does not like this:
error: unreported exception Throwable; must be caught or declared to be thrown
So I decided to add the try/catch with the null check.

I see... strange. But it does compile without the null check. I think it should be removed.

I'll remove the null check in both places where I added initCause().

reinrich · 2025-02-20T15:44:50Z

src/java.base/share/classes/com/sun/crypto/provider/DHPublicKey.java

+        } catch (IOException e) {
+            // Ignore, see JDK-8297065.
+            derKey = null;
+        }
+        return derKey.toByteArray();


This looks weired if someone has a quick look at these lines of code: if an IOE occurs and is caught here, then derKey will be assigned null and 2 lines below an NPE will thrown because of this.

I'd suggest to wrap the whole method body in a try-catch. This would reduce the diff to jdk 21.
In the IOE catch clause you shouldn't say that the IOE is ignored but you should state that it cannot even occur since DerOutputStream is a ByteArrayOutputStream which doesn't do any I/O. Then just return null or throw an InternalError are something else if more appropriate.

ok, this makes sense. See extra commit. I did it for all three places where I catch the IO exception.

reinrich · 2025-02-20T15:52:14Z

src/java.base/share/classes/com/sun/crypto/provider/DHPublicKey.java

        try {
            this.key = new DerValue(DerValue.tag_Integer,
                                    this.y.toByteArray()).toByteArray();
-            this.encodedKey = getEncoded();
+            this.encodedKey = encode(p, g, l, key);
        } catch (IOException e) {
            throw new ProviderException("Cannot produce ASN.1 encoding", e);
        }


You should remove the try-catch. encode doesn't throw IOE. This also removes the diff to jdk 21.

It is toByteArray that trows the IOException.
I can add the InternalError here as in the other places?
But here I thought it's obvious to have the old behaviour.

reinrich · 2025-02-20T15:56:12Z

src/java.base/share/classes/com/sun/crypto/provider/DHPublicKey.java

+            if (ioe != null) {
+                ioe.initCause(e);
+            }


The null check is redundant.

Suggested change

if (ioe != null) {

ioe.initCause(e);

}

ioe.initCause(e);

GoeLin · 2025-02-21T14:40:39Z

Triggered by the discussion here and offline I had a second look at the exceptions I need to add because JDK-8297065 is not in 17. I committed some code that brings these closer to the origin. One I had added is not really needed as IOException is caught at the callsite anyways.

reinrich

Looks good to me.
Cheers, Richard.

openjdk · 2025-02-21T14:59:45Z

⚠️ @GoeLin This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

reinrich · 2025-02-21T15:01:52Z

Triggered by the discussion here and offline I had a second look at the exceptions I need to add because JDK-8297065 is not in 17. I committed some code that brings these closer to the origin. One I had added is not really needed as IOException is caught at the callsite anyways.

And the diff to the versions of DHPublicKey.java and DHPrivateKey.java is minimal now.

GoeLin · 2025-02-21T16:12:06Z

Triggered by the discussion here and offline I had a second look at the exceptions I need to add because JDK-8297065 is not in 17. I committed some code that brings these closer to the origin. One I had added is not really needed as IOException is caught at the callsite anyways.

And the diff to the versions of DHPublicKey.java and DHPrivateKey.java is minimal now.

Yes, thanks for reviewing!

GoeLin · 2025-02-21T16:14:18Z

Hi @martinuy,
Although I think this is good to go now, I would appreciate a second look by you. Could you find time in the next days?
Thanks, Goetz.

GoeLin · 2025-02-25T16:19:28Z

/integrate

openjdk · 2025-02-25T16:20:23Z

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

117ce56: 8348675: TrayIcon tests fail in Ubuntu 24.10 Wayland
4216ad6: 8344581: [TESTBUG] java/awt/Robot/ScreenCaptureRobotTest.java failing on macOS
16b2146: 8339356: Test javax/net/ssl/SSLSocket/Tls13PacketSize.java failed with java.net.SocketException: An established connection was aborted by the software in your host machine
db37be3: 8342635: javax/swing/JFileChooser/FileSystemView/WindowsDefaultIconSizeTest.java creates tmp file in src dir
bb391d7: 8327476: Upgrade JLine to 3.26.1
fbf5ebe: 8198666: Many java/awt/Modal/OnTop/ test fails on mac
01d107a: 8339728: [Accessibility,Windows,JAWS] Bug in the getKeyChar method of the AccessBridge class
2846c9e: 8346828: javax/swing/JScrollBar/4865918/bug4865918.java still fails in CI
715d37e: 8346324: javax/swing/JScrollBar/4865918/bug4865918.java fails in CI
9f48d42: 8338595: Add more linesize for MIME decoder in macro bench test Base64Decode
... and 16 more: https://git.openjdk.org/jdk17u-dev/compare/2691c1428c592f952a0093e0cae3244ab48246c0...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-25T16:20:35Z

@GoeLin Pushed as commit ae0177b.

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

GoeLin added 2 commits February 12, 2025 17:30

Backport 8302111

cd891cf

Make it build, just ignore IOException. call initCause

1cff1fb

openjdk bot changed the title ~~Backport 369c573383a0120e0d85aeb89a211f38b5261013~~ 8302111: Serialization considerations Feb 17, 2025

openjdk bot added backport Port of a pull request already in a different code base rfr Pull request is ready for review labels Feb 17, 2025

reinrich reviewed Feb 19, 2025

View reviewed changes

Remove mentioning of ProviderException

55b241e

reinrich reviewed Feb 20, 2025

View reviewed changes

reinrich suggested changes Feb 20, 2025

View reviewed changes

GoeLin added 3 commits February 20, 2025 17:17

Catch exception as proposed in review

705540f

Remove null check before calling initCause

a335e4f

Bring exceptions closer to status before JDK-8297065

8317f7c

reinrich approved these changes Feb 21, 2025

View reviewed changes

openjdk bot added approval Requires approval; will be removed when approval is received ready Pull request is ready to be integrated and removed approval Requires approval; will be removed when approval is received labels Feb 24, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 25, 2025

openjdk bot closed this Feb 25, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Feb 25, 2025

openjdk bot removed the rfr Pull request is ready for review label Feb 25, 2025

GoeLin deleted the goetz_backport_8302111 branch February 25, 2025 16:20

8302111: Serialization considerations #3278

8302111: Serialization considerations #3278

Uh oh!

Conversation

GoeLin commented Feb 17, 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 Feb 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Feb 17, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

This comment was marked as resolved.

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!

GoeLin Feb 21, 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!

GoeLin commented Feb 21, 2025

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

reinrich commented Feb 21, 2025

Uh oh!

GoeLin commented Feb 21, 2025

Uh oh!

GoeLin commented Feb 21, 2025

Uh oh!

GoeLin commented Feb 25, 2025

Uh oh!

openjdk bot commented Feb 25, 2025

Uh oh!

openjdk bot commented Feb 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

GoeLin commented Feb 17, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 17, 2025 •

edited

Loading

mlbridge bot commented Feb 17, 2025 •

edited

Loading

GoeLin Feb 21, 2025 •

edited

Loading