Update doPrivilegedWithCombinerHelper function #20430

JinhangZhang · 2024-10-29T14:36:50Z

When we try to invoke doPrivilegedWithCombiner function to
perform a privileged action under an existing context
environment, we are used to construct a new context but ignore
the parent context.

We should take consideration of a combination of the current
and parent context, rather than just choose either the current
or the parent.

This patch solves the failed case in issue #19499.

JinhangZhang · 2024-11-14T18:47:18Z

@keithc-ca please help to review

JasonFengJ9 · 2024-11-14T19:19:14Z

This patch solves the race condition in issue #19499.

What's the race condition in #19499?

Are there any personal builds that pass test suites including internal compliance tests?

JinhangZhang · 2024-11-14T19:29:03Z

This patch solves the race condition in issue #19499.

What's the race condition in #19499?

Are there any personal builds that pass test suites including internal compliance tests?

Hello, the failed case in #19499 is https://github.com/ibmruntimes/openj9-openjdk-jdk23/blob/openj9/test/jdk/javax/security/auth/Subject/Compat.java#L101.

There is a personal build that pass

sanity.jck
special.jck
extended.jck
sanity.openjdk
extended.openjdk
extended.functional
sanity.functional
special.functional
extended.system
sanity.system
special.system

JasonFengJ9 · 2024-11-15T19:09:41Z

jcl/src/java.base/share/classes/java/security/AccessController.java

+	AccessControlContext parentContext = getContextHelper(context == null);
+	DomainCombiner domaincombiner = parentContext.getCombiner();
+
+	return new AccessControlContext(pdArray, domaincombiner, parentContext, context, getNewAuthorizedState(context, domain));


Suggested change

return new AccessControlContext(pdArray, domaincombiner, parentContext, context, getNewAuthorizedState(context, domain));

return new AccessControlContext(pdArray, parentContext, context, getNewAuthorizedState(context, domain));

domaincombiner = parentContext.getCombiner() can be invoked within the constructor.

It still passes both parentContext.getCombiner() and parentContext, only parentContext is needed.
In addition, pls use tabs instead of spaces to be consistent with the existing codebase.

JasonFengJ9 · 2024-11-21T15:45:54Z

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

@@ -351,6 +350,36 @@ public AccessControlContext(ProtectionDomain[] fromContext) {
 	this.containPrivilegedContext = true;
 }

+AccessControlContext(ProtectionDomain[] pdArray, AccessControlContext parent, AccessControlContext acc, int authorizeState) {


Suggested change

AccessControlContext(ProtectionDomain[] pdArray, AccessControlContext parent, AccessControlContext acc, int authorizeState) {

AccessControlContext(ProtectionDomain[] pdArray, AccessControlContext parentAcc, AccessControlContext context, int authorizeState) {

JasonFengJ9 · 2024-11-21T15:46:17Z

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

+	case STATE_AUTHORIZED:
+		if (null != acc) {
+			// when parent combiner is not null, use parent combiner to combine the current context
+			if (parent.getCombiner() != null) {


parent could be null.

JasonFengJ9 · 2024-11-21T15:47:30Z

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

+	}
+	this.doPrivilegedAcc = acc;
+	this.authorizeState = authorizeState;
+	this.containPrivilegedContext = true;


Can AccessControlContext(AccessControlContext acc, ProtectionDomain[] context, int authorizeState) invoke this constructor to avoid code duplication?

Should be in a new constructor.

Jason makes a good suggstion: the two constructors above can use the new one:

AccessControlContext(ProtectionDomain[] context, int authorizeState) { this(context, null, null, authorizeState); } AccessControlContext(AccessControlContext acc, ProtectionDomain[] context, int authorizeState) { this(context, null, acc, authorizeState); }

JasonFengJ9 · 2024-11-21T15:49:37Z

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

-/*[IF JAVA_SPEC_VERSION >= 24]*/
-	/*[MSG "K002e", "checking permissions is not supported"]*/
-	throw new AccessControlException(Msg.getString("K002e")); //$NON-NLS-1$
-/*[ELSE] JAVA_SPEC_VERSION >= 24 */


Please not remove this and other recent merged changes.

JasonFengJ9 · 2024-11-28T23:10:29Z

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

+		} else {
+			this.domainCombiner = parentAcc.domainCombiner;
+			this.context = pdArray;
+			this.nextStackAcc = parentAcc;


This changed the AccessControlContext(AccessControlContext acc, ProtectionDomain[] context, int authorizeState) behaviour.
To avoid the uncertainty of existing AccessControlContext() constructors which is being disabled permanently for JDK24+, I revert the previous suggestion. Pls separate this new constructor from others.

JasonFengJ9 · 2024-12-11T22:49:22Z

Please verify that the updated change still passes #19499 and the compliance tests.

jcl/src/java.base/share/classes/java/security/AccessController.java

keithc-ca · 2025-01-02T22:05:08Z

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

+	}
+	this.doPrivilegedAcc = acc;
+	this.authorizeState = authorizeState;
+	this.containPrivilegedContext = true;


Jason makes a good suggstion: the two constructors above can use the new one:

AccessControlContext(ProtectionDomain[] context, int authorizeState) { this(context, null, null, authorizeState); } AccessControlContext(AccessControlContext acc, ProtectionDomain[] context, int authorizeState) { this(context, null, acc, authorizeState); }

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

jcl/src/java.base/share/classes/java/security/AccessController.java

jcl/src/java.base/share/classes/java/security/AccessControlContext.java

keithc-ca · 2025-01-29T14:59:36Z

Please squash in preparation for testing.

keithc-ca · 2025-01-30T16:19:53Z

Please squash in preparation for testing.

When we try to invoke doPrivilegedWithCombiner function to perform a privileged action under an existing context environment, we are used to construct a new context but ignore the parent context. We should take consideration of a combination of the current and parent context, rather than just choose either the current or the parent. This patch solves a failed case in issue eclipse-openj9#19499. Issue: eclipse-openj9#19499 Signed-off-by: Jinhang Zhang <[email protected]>

JinhangZhang · 2025-01-31T07:09:57Z

Please squash in preparation for testing.

done

keithc-ca · 2025-01-31T16:11:06Z

Jenkins test sanity amac jdk23

JinhangZhang changed the title ~~Compat~~ Fix #https://github.com/eclipse-openj9/openj9/issues/19499 Oct 29, 2024

JinhangZhang changed the title ~~Fix #https://github.com/eclipse-openj9/openj9/issues/19499~~ Fix an openj9 failure https://github.com/eclipse-openj9/openj9/issues/19499 Oct 29, 2024

JinhangZhang changed the title ~~Fix an openj9 failure https://github.com/eclipse-openj9/openj9/issues/19499~~ Fix an openj9 failure for AccessController Oct 29, 2024

pshipton added the comp:vm label Oct 29, 2024

JinhangZhang force-pushed the compat branch 4 times, most recently from 4f2a353 to 05d9970 Compare November 12, 2024 00:12

JinhangZhang marked this pull request as ready for review November 12, 2024 00:13

JinhangZhang force-pushed the compat branch 3 times, most recently from 04afd31 to 5c436bf Compare November 12, 2024 04:48

JinhangZhang changed the title ~~Fix an openj9 failure for AccessController~~ Update doPrivilegedWithCombinerHelper function Nov 12, 2024

pshipton requested a review from JasonFengJ9 November 14, 2024 18:55

JasonFengJ9 reviewed Nov 15, 2024

View reviewed changes

JinhangZhang force-pushed the compat branch 4 times, most recently from 55d5f54 to 7bafa71 Compare November 18, 2024 04:48

JinhangZhang requested a review from JasonFengJ9 November 19, 2024 16:41

JinhangZhang force-pushed the compat branch from 7bafa71 to 18ae5e8 Compare November 20, 2024 19:55

JasonFengJ9 reviewed Nov 21, 2024

View reviewed changes

JinhangZhang force-pushed the compat branch 3 times, most recently from 29e2976 to 6d202c9 Compare November 25, 2024 03:29

JasonFengJ9 reviewed Nov 28, 2024

View reviewed changes

JinhangZhang force-pushed the compat branch from 6d202c9 to 34a73b9 Compare December 5, 2024 16:56

JinhangZhang force-pushed the compat branch 2 times, most recently from 0b44640 to 520741c Compare December 11, 2024 19:32

JasonFengJ9 requested a review from pshipton December 11, 2024 22:49

JinhangZhang force-pushed the compat branch from 243376c to 02799be Compare January 2, 2025 14:34

keithc-ca requested changes Jan 2, 2025

View reviewed changes

JinhangZhang force-pushed the compat branch 3 times, most recently from ad23095 to d51c66b Compare January 7, 2025 15:12

JinhangZhang force-pushed the compat branch 2 times, most recently from a992864 to 9dd994d Compare January 12, 2025 08:20

keithc-ca requested changes Jan 13, 2025

View reviewed changes

jcl/src/java.base/share/classes/java/security/AccessControlContext.java Outdated Show resolved Hide resolved

jcl/src/java.base/share/classes/java/security/AccessController.java Outdated Show resolved Hide resolved

JinhangZhang force-pushed the compat branch 2 times, most recently from ba0a69c to 041311e Compare January 20, 2025 18:08

JinhangZhang requested review from keithc-ca and JasonFengJ9 January 20, 2025 19:50

keithc-ca requested changes Jan 22, 2025

View reviewed changes

jcl/src/java.base/share/classes/java/security/AccessControlContext.java Outdated Show resolved Hide resolved

jcl/src/java.base/share/classes/java/security/AccessControlContext.java Outdated Show resolved Hide resolved

JinhangZhang requested a review from keithc-ca January 28, 2025 15:39

JinhangZhang force-pushed the compat branch from 041311e to 4fe4bed Compare January 29, 2025 03:51

JinhangZhang force-pushed the compat branch from 4fe4bed to 5b2181e Compare January 29, 2025 18:25

pshipton removed their request for review January 30, 2025 17:28

JinhangZhang force-pushed the compat branch from 5b2181e to eed4ec4 Compare January 31, 2025 07:09

keithc-ca approved these changes Jan 31, 2025

View reviewed changes

keithc-ca merged commit 24960fd into eclipse-openj9:master Jan 31, 2025
6 checks passed

JasonFengJ9 mentioned this pull request Feb 20, 2025

JDK23 jdk_security3_0_FAILED javax/security/auth/Subject/Compat.java doPrivWC on a2: Expect three, but see two #19499

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update doPrivilegedWithCombinerHelper function #20430

Update doPrivilegedWithCombinerHelper function #20430

JinhangZhang commented Oct 29, 2024 •

edited

Loading

JinhangZhang commented Nov 14, 2024

JasonFengJ9 commented Nov 14, 2024

JinhangZhang commented Nov 14, 2024 •

edited

Loading

JasonFengJ9 Nov 15, 2024

JinhangZhang Nov 18, 2024

JasonFengJ9 Nov 19, 2024

JinhangZhang Nov 20, 2024

JasonFengJ9 Nov 21, 2024

JinhangZhang Nov 22, 2024

JasonFengJ9 Nov 21, 2024

JinhangZhang Jan 12, 2025

JasonFengJ9 Nov 21, 2024

JinhangZhang Dec 11, 2024

keithc-ca Jan 2, 2025

JinhangZhang Jan 12, 2025

JasonFengJ9 Nov 21, 2024

JinhangZhang Nov 22, 2024

JasonFengJ9 Nov 28, 2024

JinhangZhang Dec 11, 2024

JasonFengJ9 commented Dec 11, 2024

keithc-ca Jan 2, 2025

keithc-ca commented Jan 29, 2025

keithc-ca commented Jan 30, 2025

JinhangZhang commented Jan 31, 2025

keithc-ca commented Jan 31, 2025

	return new AccessControlContext(pdArray, domaincombiner, parentContext, context, getNewAuthorizedState(context, domain));
	return new AccessControlContext(pdArray, parentContext, context, getNewAuthorizedState(context, domain));

	AccessControlContext(ProtectionDomain[] pdArray, AccessControlContext parent, AccessControlContext acc, int authorizeState) {
	AccessControlContext(ProtectionDomain[] pdArray, AccessControlContext parentAcc, AccessControlContext context, int authorizeState) {

Update doPrivilegedWithCombinerHelper function #20430

Update doPrivilegedWithCombinerHelper function #20430

Conversation

JinhangZhang commented Oct 29, 2024 • edited Loading

JinhangZhang commented Nov 14, 2024

JasonFengJ9 commented Nov 14, 2024

JinhangZhang commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonFengJ9 commented Dec 11, 2024

Choose a reason for hiding this comment

keithc-ca commented Jan 29, 2025

keithc-ca commented Jan 30, 2025

JinhangZhang commented Jan 31, 2025

keithc-ca commented Jan 31, 2025

JinhangZhang commented Oct 29, 2024 •

edited

Loading

JinhangZhang commented Nov 14, 2024 •

edited

Loading