openjdk
8357915: SecureRandom nextLong memory usage
SecureRandom uses straightforward implementations inherited from Random but in the process does double the memory allocations necessary.
The delegation to SecureRandom.engineNextBytes does not provide int or long values, the caller must allocate a byte array and assemble the value itself.
So added an implementation in SecureRandom that call nextBytes(8 bytes) and then convert that to a long.
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8357915: SecureRandom nextLong memory usage (Enhancement - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26005/head:pull/26005
$ git checkout pull/26005
Update a local copy of the PR:
$ git checkout pull/26005
$ git pull https://git.openjdk.org/jdk.git pull/26005/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26005
View PR using the GUI difftool:
$ git pr show -t 26005
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26005.diff
:wave: Welcome back koushikthirupattur! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@koushikthirupattur 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:
8357915: SecureRandom nextLong memory usage
Reviewed-by: wetmore
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 120 new commits pushed to the master branch:
- 92712ef45dd81fa9f03fbd6427f8c1507f28e62b: 8361367: AOT ExcludedClasses.java test failed with missing constant pool logs
- 5850bf4488ea336c3dd4eafbefb8ade330e2f76a: 8361519: Obsolete Unicode Scalar Value link in Character class
- 853319439e7887ddd54f8c4a3d79aa62ec51fd64: 8361570: Incorrect 'sealed is not allowed here' compile-time error
- ... and 117 more: https://git.openjdk.org/jdk/compare/8ea544c33fc502208577249fb83544f8d876bc17...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 (@bradfordwetmore) 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).
@koushikthirupattur 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.
@seanjmullan has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@koushikthirupattur please create a CSR request for issue JDK-8357915 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
The suggestion in the issue was to switch to RandomGenerator.nextLong() can you provide an explanation as to why that is not an acceptable solution. It would avoid any allocation. Thanks
@koushikthirupattur can you please add a regression test to provide coverage for nextLong?
The suggestion in the issue was to switch to RandomGenerator.nextLong() can you provide an explanation as to why that is not an acceptable solution. It would avoid any allocation. Thanks
Based on our discussion, while RandomGenerator.nextLong() does avoid allocation, we are Overriding nextLong() locally in SecureRandom because its minimal, backward-compatible, behavior-preserving, secure. Thanks!
@koushikthirupattur can you please add a regression test to provide coverage for
nextLong?
Sure, we can add one for coverage but I wanted to learn if we really need a regression test in this case as there is no behavior change to test against and this fix only improves internal memory allocation in SecureRandom.nextLong(). Let me know.
@seanjmullan The CSR requirement cannot be removed as CSR issues already exist. Please withdraw JDK-8361100 and then use the command /csr unneeded again.
:warning: @koushikthirupattur the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:
$ git checkout 8357915
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push
@koushikthirupattur Your change (at version 04f859bdec18f767bf33156939a02f4a336c42e2) is now ready to be sponsored by a Committer.
Going to push as commit 91df7978799e5a24a73d8e1ae344e532e572f2dd.
Since your change was applied there have been 120 commits pushed to the master branch:
- 92712ef45dd81fa9f03fbd6427f8c1507f28e62b: 8361367: AOT ExcludedClasses.java test failed with missing constant pool logs
- 5850bf4488ea336c3dd4eafbefb8ade330e2f76a: 8361519: Obsolete Unicode Scalar Value link in Character class
- 853319439e7887ddd54f8c4a3d79aa62ec51fd64: 8361570: Incorrect 'sealed is not allowed here' compile-time error
- ... and 117 more: https://git.openjdk.org/jdk/compare/8ea544c33fc502208577249fb83544f8d876bc17...master
Your commit was automatically rebased without conflicts.
@bradfordwetmore @koushikthirupattur Pushed as commit 91df7978799e5a24a73d8e1ae344e532e572f2dd.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.