Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264681: Use the blessed modifier order in java.security #3338

Closed
wants to merge 2 commits into from

Conversation

@alblue
Copy link
Contributor

@alblue alblue commented Apr 3, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264681: Use the blessed modifier order in java.security

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3338/head:pull/3338
$ git checkout pull/3338

Update a local copy of the PR:
$ git checkout pull/3338
$ git pull https://git.openjdk.java.net/jdk pull/3338/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3338

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3338.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 3, 2021

👋 Welcome back alblue! 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
Copy link

@openjdk openjdk bot commented Apr 3, 2021

@alblue 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
Copy link

@mlbridge mlbridge bot commented Apr 3, 2021

Webrevs

Copy link
Member

@seanjmullan seanjmullan left a comment

The rest looks fine, but I would double-check all the copyrights to see if you are modifying any other 3rd-party code than the ones I commented on. Best to leave that code as-is and fix it at the source, if possible.

@@ -50,7 +50,7 @@


/**
* This class holds only final static member variables that are constants
* This class holds only static final member variables that are constants

This comment has been minimized.

@seanjmullan

seanjmullan Apr 8, 2021
Member

Probably best to leave this file unchanged as it is 3rd-party code.

This comment has been minimized.

@shipilev

shipilev Apr 12, 2021
Contributor

Same here...

@@ -387,7 +387,7 @@ private static void checkKeySize(XMLCryptoContext context, Key key)
super(dmElem);
}

abstract public PSSParameterSpec getPSSParameterSpec();
public abstract PSSParameterSpec getPSSParameterSpec();

This comment has been minimized.

@seanjmullan

seanjmullan Apr 8, 2021
Member

This is 3rd-party (Apache) code. It would be better to change this at Apache. I would leave this one out.

This comment has been minimized.

@shipilev

shipilev Apr 12, 2021
Contributor

@alblue, this is the unresolved comment. Please fix it?

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

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

8264681: Use the blessed modifier order in java.security

Reviewed-by: mullan, shade

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

  • 784f1c1: 8264200: java/nio/channels/DatagramChannel/SRTest.java fails intermittently
  • a4f644e: 8265064: Move clearing and setting of members into helpers in ReservedSpace
  • 7006070: 8265061: Simplify MethodHandleNatives::canBeCalledVirtual
  • 1935655: 8264957: Cleanup unused array Type::dual_type
  • 954b9a1: 8264795: IGV: Upgrade NetBeans platform
  • f2f7aa3: 8262291: Refactor reserve_memory_special_huge_tlbfs
  • 008fc75: 8264224: Add macosx-aarch64 to Oracle build configurations
  • f4e6395: 8264190: Harden TLS interop tests
  • 18bec9c: 8265084: [BACKOUT] 8264954: unified handling for VectorMask object re-materialization during de-optimization
  • 9dd9625: 8263763: Synthetic constructor parameters of enum are not considered for annotation indices
  • ... and 101 more: https://git.openjdk.java.net/jdk/compare/f259eeaf65cbbd360e96f8c102653feb636eb899...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@seanjmullan, @shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Apr 8, 2021
@alblue
Copy link
Contributor Author

@alblue alblue commented Apr 10, 2021

/integrate

@openjdk openjdk bot added the sponsor label Apr 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 10, 2021

@alblue
Your change (at version d91bdba) is now ready to be sponsored by a Committer.

Copy link
Contributor

@shipilev shipilev left a comment

I think some review comments from Sean were left unaddressed...

@@ -387,7 +387,7 @@ private static void checkKeySize(XMLCryptoContext context, Key key)
super(dmElem);
}

abstract public PSSParameterSpec getPSSParameterSpec();
public abstract PSSParameterSpec getPSSParameterSpec();

This comment has been minimized.

@shipilev

shipilev Apr 12, 2021
Contributor

@alblue, this is the unresolved comment. Please fix it?

@@ -50,7 +50,7 @@


/**
* This class holds only final static member variables that are constants
* This class holds only static final member variables that are constants

This comment has been minimized.

@shipilev

shipilev Apr 12, 2021
Contributor

Same here...

@alblue
Copy link
Contributor Author

@alblue alblue commented Apr 12, 2021

Sorry, thought those changes had been removed. Let me fix.

@openjdk openjdk bot removed the sponsor label Apr 12, 2021
@alblue
Copy link
Contributor Author

@alblue alblue commented Apr 12, 2021

Did a search of the files included in the commit, didn't see anything other than the two above without the standard Oracle copyright .

Wasn't sure about src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_ATTRIBUTE.java though; it has an Oracle copyright on it (and not the 'do not alter') one. Should I remove that one from this commit as well?

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Apr 12, 2021

Did a search of the files included in the commit, didn't see anything other than the two above without the standard Oracle copyright .

Wasn't sure about src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_ATTRIBUTE.java though; it has an Oracle copyright on it (and not the 'do not alter') one. Should I remove that one from this commit as well?

This one should be ok. We have already made changes to this file so preserving pure parity with the 3rd party sources is not an overriding concern.

@alblue
Copy link
Contributor Author

@alblue alblue commented Apr 13, 2021

What happens to this now? Do I need to wait for a re-review of the change or do I hit the (slash) integrate command now?

Copy link
Contributor

@shipilev shipilev left a comment

I think all conversations have been resolved, we can integrate.

@alblue
Copy link
Contributor Author

@alblue alblue commented Apr 13, 2021

/integrate

@openjdk openjdk bot added the sponsor label Apr 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 13, 2021

@alblue
Your change (at version 3e9047f) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Apr 14, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2021

@shipilev @alblue Since your change was applied there have been 125 commits pushed to the master branch:

  • ea5c55a: 8265103: Remove unnecessary inclusion of oopMap.hpp
  • 26186ec: 8039261: [TEST_BUG]: There is not a minimal security level in Java Preferences and the TestApplet.html is blocked.
  • 283d64f: 8262896: [macos_aarch64] Crash in jni_fast_GetLongField
  • 55d5649: 8263157: [macos]: java.library.path is being set incorrectly
  • e80012e: 8264768: JFR: Allow events to be printed to the log
  • 3b576ed: 8265100: (fs) WindowsFileStore.hashCode() should read cached hash code once
  • 8df8512: 8265125: IGV: cannot edit forms with NetBeans GUI builder
  • 9cd5400: 8265138: Simplify DerUtils::checkAlg
  • c797511: 8264940: java/lang/invoke/6998541/Test6998541.java failed "guarantee(ik->is_initialized()) failed: java/lang/Byte$ByteCache must be initialized"
  • 943503e: 8265035: Remove unneeded exception check from refill_ic_stubs()
  • ... and 115 more: https://git.openjdk.java.net/jdk/compare/f259eeaf65cbbd360e96f8c102653feb636eb899...master

Your commit was automatically rebased without conflicts.

Pushed as commit ebbce91.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants