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

8259021: SharedSecrets should avoid double racy reads from non-volatile fields #1914

Closed

Conversation

@plevart
Copy link
Contributor

@plevart plevart commented Dec 31, 2020

See: https://bugs.openjdk.java.net/browse/JDK-8259021
See also discussion in thread: https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-December/072798.html


Progress

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

Issue

  • JDK-8259021: SharedSecrets should avoid double racy reads from non-volatile fields

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1914/head:pull/1914
$ git checkout pull/1914

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 31, 2020

👋 Welcome back plevart! 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 openjdk bot added the rfr label Dec 31, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 31, 2020

@plevart The following labels will be automatically applied to this pull request:

  • core-libs
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 31, 2020

Webrevs

Copy link
Contributor

@shipilev shipilev left a comment

Looks good, but can we not do the behavioral change in ensureClassInitialized? There are methods like this in JDK, which could probably be changed wholesale?

} catch (IllegalAccessException e) {
throw new InternalError(e);
}
Comment on lines 417 to 419

This comment has been minimized.

@shipilev

shipilev Jan 4, 2021
Contributor

This is a potential behavioral change that has little to do with the issue at hand, right?

@cl4es
cl4es approved these changes Jan 4, 2021
Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

The bug title and the PR title need to be the same.
Editing either one is fine.

@reinrich
Copy link
Contributor

@reinrich reinrich commented Jan 4, 2021

But wouldn't it be legal for a compiler (java to bytecode or bytecode to
machinecode) to replace references of my_local_copy with references to
static_field?

Foo my_local_copy = static_field;
if (my_copy == null) {
   initialize();
   my_local_copy = static_field;
}
return my_local_copy;

Only if static_field was volatile this would be illegal, wouldn't it?

@plevart plevart changed the title 8259021 avoid double racy reads from non-volatile fields of SharedSecrets 8259021: SharedSecrets should avoid double racy reads from non-volatile fields Jan 4, 2021
@plevart
Copy link
Contributor Author

@plevart plevart commented Jan 4, 2021

@reinrich I don't think Java compilers may do that. If this was allowed, such variables would not be called "local".

@openjdk
Copy link

@openjdk openjdk bot commented Jan 4, 2021

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

8259021: SharedSecrets should avoid double racy reads from non-volatile fields

Reviewed-by: shade, redestad, rriggs, mchung, rrich, alanb

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

  • dd8996c: 8258946: Fix optimization-unstable code involving signed integer overflow
  • 5ea9607: 8258459: Decouple gc_globals.hpp from globals.hpp
  • 2499ac3: 8259069: Fields could be final
  • f0aae81: 8259020: null-check of g1 write_ref_field_pre_entry is not necessary
  • e6f9926: 8257809: JNI warnings from Toolkit JPEG image decoding
  • 9d160aa: 8257815: Replace global log2 functions with efficient implementations
  • f80c63b: 8258490: Shenandoah: Full GC does not need to remark threads and drain SATB buffers
  • 7f04d23: 8258800: Deprecate -XX:+AlwaysLockClassLoader
  • d679caa: 8196466: javax/swing/JFileChooser/8062561/bug8062561.java fails
  • a2a3f4a: 8258924: javax/swing/JSplitPane/4201995/bug4201995.java fails in GTk L&F
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/07c93fab8506b844e3689fad92ec355ab0dd3c54...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 openjdk bot added the ready label Jan 4, 2021
@mlchung
mlchung approved these changes Jan 4, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 4, 2021

Mailing list message from Hans Boehm on core-libs-dev:

On Mon, Jan 4, 2021 at 8:34 AM Peter Levart <plevart at openjdk.java.net>
wrote:

On Mon, 4 Jan 2021 15:57:33 GMT, Richard Reingruber <rrich at openjdk.org>

wrote:

The bug title and the PR title need to be the same.
Editing either one is fine.

But wouldn't it be legal for a compiler (java to bytecode or bytecode to
machinecode) to replace references of my_local_copy with references to
static_field?

Foo my_local_copy = static_field;
if (my_copy == null) {
initialize();
my_local_copy = static_field;
}
return my_local_copy;

Only if static_field was volatile this would be illegal, wouldn't it?

@reinrich I don't think Java compilers may do that. If this was allowed,

such variables would not be called "local".

Indeed. Such transformations are allowed in C and C++ (since data races
result in undefined behavior, and thus the
compiler is allowed to assume there are no concurrent writes), but not in
Java.

@reinrich
Copy link
Contributor

@reinrich reinrich commented Jan 4, 2021

Mailing list message from Hans Boehm on core-libs-dev:

On Mon, Jan 4, 2021 at 8:34 AM Peter Levart
wrote:

On Mon, 4 Jan 2021 15:57:33 GMT, Richard Reingruber

wrote:

The bug title and the PR title need to be the same.
Editing either one is fine.

But wouldn't it be legal for a compiler (java to bytecode or bytecode to
machinecode) to replace references of my_local_copy with references to
static_field?
Foo my_local_copy = static_field;
if (my_copy == null) {
initialize();
my_local_copy = static_field;
}
return my_local_copy;
Only if static_field was volatile this would be illegal, wouldn't it?

@reinrich I don't think Java compilers may do that. If this was allowed,

such variables would not be called "local".

Indeed. Such transformations are allowed in C and C++ (since data races
result in undefined behavior, and thus the
compiler is allowed to assume there are no concurrent writes), but not in
Java.

Thanks for the explanation.

@plevart
Copy link
Contributor Author

@plevart plevart commented Jan 5, 2021

/integrate

@openjdk openjdk bot closed this Jan 5, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 5, 2021

@plevart Since your change was applied there have been 24 commits pushed to the master branch:

  • d5aa49d: 8259236: C2 compilation fails with assert(is_power_of_2(value)) failed: value must be a power of 2: 8000000000000000
  • 82bdbfd: 8258857: Zero: non-PCH release build fails after JDK-8258074
  • f4122d6: 8258896: Remove the JVM ForceFloatExceptions option
  • fc3b45c: 8258643: javax/swing/JComponent/7154030/bug7154030.java failed with "Exception: Failed to hide opaque button"
  • a6c0881: 8256321: Some "inactive" color profiles use the wrong profile class
  • 9f15164: 8259049: Uninitialized variable after JDK-8257513
  • db6f393: 8251944: Add Shenandoah test config to compiler/gcbarriers/UnsafeIntrinsicsTest.java
  • 3817c32: 8258534: Epsilon: clean up unused includes
  • 17d1645: 8258751: Improve ExceptionHandlerTable dump
  • dd8996c: 8258946: Fix optimization-unstable code involving signed integer overflow
  • ... and 14 more: https://git.openjdk.java.net/jdk/compare/07c93fab8506b844e3689fad92ec355ab0dd3c54...master

Your commit was automatically rebased without conflicts.

Pushed as commit 85bac8c.

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

@plevart plevart deleted the plevart:8259021_avoid_double_racy_reads branch Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment