Re: RFR: 8334394: Race condition in Class::protectionDomain [v3]
On Mon, 22 Jul 2024 12:42:48 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with two additional > commits since the last revision: > > - var to real type > - rename Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2191592473
Re: RFR: 8334394: Race condition in Class::protectionDomain [v3]
On Mon, 22 Jul 2024 12:42:48 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with two additional > commits since the last revision: > > - var to real type > - rename Marked as reviewed by liach (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2191574416
Re: RFR: 8334394: Race condition in Class::protectionDomain [v3]
On Mon, 22 Jul 2024 12:42:48 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with two additional > commits since the last revision: > > - var to real type > - rename Still looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2191565356
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Tue, 16 Jul 2024 12:59:36 GMT, Alan Bateman wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> explain why the test is related to the fix
>
> test/jdk/java/lang/Class/ProtectionDomainRace.java line 61:
>
>> 59: private static volatile Throwable failed = null;
>> 60: public static void main(String[] args) throws Throwable {
>> 61: var ac = (PrivilegedAction) () -> null;
>
> Did you mean to name the PrivilegedAction "ac"? It might be more readable to
> change this line to `PrivilegedAction pa = () -> null;`.
Done. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1686482409
Re: RFR: 8334394: Race condition in Class::protectionDomain [v3]
> Make sure `pd` is always the same object when `getProtectionDomain0` is null. Weijun Wang has updated the pull request incrementally with two additional commits since the last revision: - var to real type - rename - Changes: - all: https://git.openjdk.org/jdk/pull/19752/files - new: https://git.openjdk.org/jdk/pull/19752/files/4ce5bc24..ed2ba88a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19752&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19752&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19752/head:pull/19752 PR: https://git.openjdk.org/jdk/pull/19752
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > explain why the test is related to the fix lgtm - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2186068679
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote:
>> Make sure `pd` is always the same object when `getProtectionDomain0` is null.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> explain why the test is related to the fix
Marked as reviewed by alanb (Reviewer).
test/jdk/java/lang/Class/ProtectionDomainRace.java line 61:
> 59: private static volatile Throwable failed = null;
> 60: public static void main(String[] args) throws Throwable {
> 61: var ac = (PrivilegedAction) () -> null;
Did you mean to name the PrivilegedAction "ac"? It might be more readable to
change this line to `PrivilegedAction pa = () -> null;`.
-
PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2180164683
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1679348512
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > explain why the test is related to the fix Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2174075188
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > explain why the test is related to the fix Marked as reviewed by liach (Committer). I think the move of protectionDomain and signers fields from injected to explicit should be done in a separate RFE: created https://bugs.openjdk.org/browse/JDK-8334772 to track that. - PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2134339047 PR Comment: https://git.openjdk.org/jdk/pull/19752#issuecomment-2185147789
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > explain why the test is related to the fix The update to Class looks okay. At some point I think we see if the protection domain field can be changed to be an explicit field (not injected) so that the native method and JVM_GetProtectionDomain can be removed. - PR Comment: https://git.openjdk.org/jdk/pull/19752#issuecomment-2173980108
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
> Make sure `pd` is always the same object when `getProtectionDomain0` is null. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: explain why the test is related to the fix - Changes: - all: https://git.openjdk.org/jdk/pull/19752/files - new: https://git.openjdk.org/jdk/pull/19752/files/637cf0df..4ce5bc24 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19752&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19752&range=00-01 Stats: 25 lines in 1 file changed: 25 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19752/head:pull/19752 PR: https://git.openjdk.org/jdk/pull/19752
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 15:11:29 GMT, Weijun Wang wrote:
>> test/jdk/java/lang/Class/ProtectionDomainRace.java line 42:
>>
>>> 40: try {
>>> 41: Subject.doAs(null, ac);
>>> 42: } catch (Throwable t) {
>>
>> This can test for the racy NPE, but it cannot detect if there's distinct
>> allPermDomain objects written by races. Should we add another check to
>> defend against that?
>
> Actually it can. This bug originally breaks the `assert` statement at
> https://github.com/openjdk/jdk/blob/5cea53d372744ddf1bedaae4667415e6525ef82f/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#L1209.
So this test might look like unrelated to the fix at first sight, but somewhere
deep inside the call stack it does. I should add a comment. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1642978163
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 15:08:26 GMT, Chen Liang wrote:
>> Make sure `pd` is always the same object when `getProtectionDomain0` is null.
>
> test/jdk/java/lang/Class/ProtectionDomainRace.java line 42:
>
>> 40: try {
>> 41: Subject.doAs(null, ac);
>> 42: } catch (Throwable t) {
>
> This can test for the racy NPE, but it cannot detect if there's distinct
> allPermDomain objects written by races. Should we add another check to defend
> against that?
Actually it can. This bug originally breaks the `assert` statement at
https://github.com/openjdk/jdk/blob/5cea53d372744ddf1bedaae4667415e6525ef82f/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#L1209.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1642973826
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 14:51:07 GMT, Weijun Wang wrote:
> Make sure `pd` is always the same object when `getProtectionDomain0` is null.
test/jdk/java/lang/Class/ProtectionDomainRace.java line 42:
> 40: try {
> 41: Subject.doAs(null, ac);
> 42: } catch (Throwable t) {
This can test for the racy NPE, but it cannot detect if there's distinct
allPermDomain objects written by races. Should we add another check to defend
against that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19752#discussion_r1642969479
Re: RFR: 8334394: Race condition in Class::protectionDomain
On Mon, 17 Jun 2024 14:51:07 GMT, Weijun Wang wrote: > Make sure `pd` is always the same object when `getProtectionDomain0` is null. Looks great! I just wonder about how we usually write tests for such races, as these races may be platform-dependent and the test might not be able to always reproduce the race (such as with number of threads). - PR Comment: https://git.openjdk.org/jdk/pull/19752#issuecomment-2173658541
RFR: 8334394: Race condition in Class::protectionDomain
Make sure `pd` is always the same object when `getProtectionDomain0` is null. - Commit messages: - chmod - the fix Changes: https://git.openjdk.org/jdk/pull/19752/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19752&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334394 Stats: 84 lines in 2 files changed: 67 ins; 10 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19752/head:pull/19752 PR: https://git.openjdk.org/jdk/pull/19752
