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

8301578: Perform output outside synchronization in Module.class #12193

Closed
wants to merge 11 commits into from
78 changes: 42 additions & 36 deletions src/java.base/share/classes/java/lang/Module.java
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -57,6 +57,7 @@
import jdk.internal.loader.BootLoader;
import jdk.internal.loader.ClassLoaders;
import jdk.internal.misc.CDS;
import jdk.internal.misc.Unsafe;
import jdk.internal.module.ModuleBootstrap;
import jdk.internal.module.ModuleLoaderMap;
import jdk.internal.module.ServicesCatalog;
Expand Down Expand Up @@ -113,6 +114,8 @@ public final class Module implements AnnotatedElement {
private final ModuleDescriptor descriptor;

// true, if this module allows restricted native access
// Accessing this variable is made through Unsafe in order to use the
// memory semantics that preserves ordering and visibility across threads.
@Stable
private boolean enableNativeAccess;

Expand Down Expand Up @@ -258,24 +261,43 @@ public ModuleLayer getLayer() {
/**
* Update this module to allow access to restricted methods.
*/
synchronized Module implAddEnableNativeAccess() {
enableNativeAccess = true;
Module implAddEnableNativeAccess() {
EnableNativeAccess.trySetEnableNativeAccess(this);
return this;
}

/**
* Returns {@code true} if this module can access
* <a href="foreign/package-summary.html#restricted"><em>restricted</em></a> methods.
*
* @since 20
*
* @return {@code true} if this module can access <em>restricted</em> methods.
* @since 20
*/
@PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)
@PreviewFeature(feature = PreviewFeature.Feature.FOREIGN)
public boolean isNativeAccessEnabled() {
Module target = moduleForNativeAccess();
synchronized(target) {
return target.enableNativeAccess;
return EnableNativeAccess.isNativeAccessEnabled(target);
}

/**
* This class is used to be able to bootstrap without using Unsafe
* in the outer Module class as that would create a circular initializer dependency.
*/
private static final class EnableNativeAccess {

private EnableNativeAccess() {}

private static final Unsafe UNSAFE = Unsafe.getUnsafe();
private static final long FIELD_OFFSET = UNSAFE.objectFieldOffset(Module.class, "enableNativeAccess");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Unsafe and CAS'ing the enableNativeAccess field looks okay. The name "AccessHolder" looks too general here as there is a lot of access going on in this class, maybe NativeAccessHolder will work.

It would be good if you could try to keep changes to this code consistent with the existing style if possible. In this case, the other inner class uses /** .. */ style comment for the class description. I think would I put the constants at the top of the this class.


private static boolean isNativeAccessEnabled(Module target) {
return UNSAFE.getBooleanVolatile(target, FIELD_OFFSET);
}

// Atomically sets enableNativeAccess if not already set
// returning if the value was updated
private static boolean trySetEnableNativeAccess(Module target) {
return UNSAFE.compareAndSetBoolean(target, FIELD_OFFSET, false, true);
}
}

Expand All @@ -289,46 +311,30 @@ private Module moduleForNativeAccess() {
void ensureNativeAccess(Class<?> owner, String methodName) {
// The target module whose enableNativeAccess flag is ensured
Module target = moduleForNativeAccess();
// racy read of the enable native access flag
boolean isNativeAccessEnabled = target.enableNativeAccess;
if (!isNativeAccessEnabled) {
synchronized (target) {
// safe read of the enableNativeAccess of the target module
isNativeAccessEnabled = target.enableNativeAccess;

// check again with the safely read flag
if (isNativeAccessEnabled) {
// another thread beat us to it - nothing to do
return;
} else if (ModuleBootstrap.hasEnableNativeAccessFlag()) {
throw new IllegalCallerException("Illegal native access from: " + this);
} else {
// warn and set flag, so that only one warning is reported per module
String cls = owner.getName();
String mtd = cls + "::" + methodName;
String mod = isNamed() ? "module " + getName() : "the unnamed module";
String modflag = isNamed() ? getName() : "ALL-UNNAMED";
System.err.printf("""
if (!EnableNativeAccess.isNativeAccessEnabled(target)) {
if (ModuleBootstrap.hasEnableNativeAccessFlag()) {
throw new IllegalCallerException("Illegal native access from: " + this);
}
if (EnableNativeAccess.trySetEnableNativeAccess(target)) {
// warn and set flag, so that only one warning is reported per module
String cls = owner.getName();
String mtd = cls + "::" + methodName;
String mod = isNamed() ? "module " + getName() : "the unnamed module";
String modflag = isNamed() ? getName() : "ALL-UNNAMED";
System.err.printf("""
WARNING: A restricted method in %s has been called
WARNING: %s has been called by %s
WARNING: Use --enable-native-access=%s to avoid a warning for this module
%n""", cls, mtd, mod, modflag);

// set the flag
target.enableNativeAccess = true;
}
}
}
}


/**
* Update all unnamed modules to allow access to restricted methods.
*/
static void implAddEnableNativeAccessToAllUnnamed() {
synchronized (ALL_UNNAMED_MODULE) {
ALL_UNNAMED_MODULE.enableNativeAccess = true;
}
EnableNativeAccess.trySetEnableNativeAccess(ALL_UNNAMED_MODULE);
}

// --
Expand Down