On 21/02/2019 16:49, Roger Riggs wrote:

Hi Andrew,

I can sponsor; it looks correct to me.

Any other reviewers? It looks right but would be good to have a test case that demonstrates the issue.

-Alan

Hi Roger, I had a think about this and a testcase will be difficult, as it was found during OpenJ9 testing and occured during VM bootstrap, feel free to read further details here: https://github.com/eclipse/openj9/issues/3399#issuecomment-459004840 So the issue was discovered due to the bootstrap behaviour, it was not observed with Hotspot, however given the obvious missing initialization check logic in the class it's not to say there's an untested route with Hotspot that could hit it... I'm not sure how I could scaffold a jtreg test to replicate the same? Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leonard@uk.ibm.com From: Roger Riggs <Roger.Riggs@oracle.com> To: core-libs-dev@openjdk.java.net, Andrew Leonard <andrew_m_leonard@uk.ibm.com> Date: 21/02/2019 18:10 Subject: Re: Fix proposal: JDK-8219378 NPE in ReflectionFactory.newMethodAccessor when langReflectAccess not initialized Hi Andrew, Is there a test case? Since the initialization is a side effect of static initialization, it might be hard to trigger just one of those paths. How was it discovered? Thanks, Roger On 02/21/2019 11:57 AM, Alan Bateman wrote: On 21/02/2019 16:49, Roger Riggs wrote: Hi Andrew, I can sponsor; it looks correct to me. Any other reviewers? It looks right but would be good to have a test case that demonstrates the issue. -Alan Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

On 22/02/2019 08:49, Andrew Leonard wrote:

Hi Roger, I had a think about this and a testcase will be difficult, as it was found during OpenJ9 testing and occured during VM bootstrap, Can you say if the J9 failure was with an exploded build or an images build? Just asking as suggest we could trigger this with an exploded build during startup with the right test. If it's too much work then no problem, I agree with Roger that the issue is obvious.

-Alan

Hi Andrew, Thanks for the stack trace of the issue triggering this. It seems to me that Modifier::<clinit> isn't the right place to setLangReflectAccess shared secret. It might have assumed that Modifier should have been initialized when Field/Method or other AccessibleObject is instantiated which isn't true since the modifiers field is an int. While the fix looks okay, would you mind trying a different fix moving Modifier::<clinit> to AccessibleObject? See this resolves the issue when running on J9? Mandy On 2/22/19 12:49 AM, Andrew Leonard wrote:

Hi Roger, I had a think about this and a testcase will be difficult, as it was found during OpenJ9 testing and occured during VM bootstrap, feel free to read further details here: https://github.com/eclipse/openj9/issues/3399#issuecomment-459004840 So the issue was discovered due to the bootstrap behaviour, it was not observed with Hotspot, however given the obvious missing initialization check logic in the class it's not to say there's an untested route with Hotspot that could hit it... I'm not sure how I could scaffold a jtreg test to replicate the same? Thanks Andrew

On 2/25/19 5:12 AM, Andrew Leonard wrote:

Hi Mandy, I must admit I don't completely follow the logic of the existing Modifier init of langReflectAccess, the comment indicates a "protocol between java.lang and java.lang.reflect".

That sets up the shared secret for ReflectionFactory to access non-public members in java.lang.reflect.

I can try moving the clinit code from Modifier to AccessibleObject, but I question is there some reason it is there? Would we be sure in moving it we are not missing something?

ReflectionFactory is the internal support for reflection. The methods that access LangReflectAccess shared secrets should have a Method, Field or Constructor in hand. The ReflectionFactory::newField and newMethod that don't take Field/Method parameter are unused (I suspect they were used by the VM native reflection implementation previously. That led me to suggest to move setLangReflectAccess to AccessibleObject. AccessibleObject is initialized very early during startup by the VM. My proposed fix would change the list of classes loaded during early startup but it would need to look at closely. Having a second thought, my proposed fix can be a follow-on clean up. I'm okay with your point fix that resolves JDK-8219378. I will file a JBS issue for the follow-on clean up. What do you think? thanks Mandy

Hi Andrew, I've got the changeset you sent earlier and have tested it as well. I'll push it today unless there any objections. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-reflectionfactory-8219378-2/ Roger On 02/26/2019 11:46 AM, Andrew Leonard wrote:

Hi Mandy, I think your last proposal sounds a good plan, push 8219378 to fix the immediate issue and create a new change to refactor the clinit location... As I am not a "Committer" (yet! working on it!) I can't use the Submit repo. I have built your patch and run a bunch of tests locally and it works great. I have also tested it successfully with J9. So I am happy with your change.

If Roger & yourself are happy with 8219378, can we Submit-repo it for final test and get that one merged? Many thanks Andrew

Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leonard@uk.ibm.com

From: Mandy Chung <mandy.chung@oracle.com> To: Andrew Leonard <andrew_m_leonard@uk.ibm.com> Cc: core-libs-dev@openjdk.java.net, Roger Riggs <Roger.Riggs@oracle.com> Date: 26/02/2019 00:16 Subject: Re: Fix proposal: JDK-8219378 NPE in ReflectionFactory.newMethodAccessor when langReflectAccess not initialized ------------------------------------------------------------------------

On 2/25/19 5:12 AM, Andrew Leonard wrote:

...

Hi Mandy, I must admit I don't completely follow the logic of the existing Modifier init of langReflectAccess, the comment indicates a "protocol between java.lang and java.lang.reflect".

That sets up the shared secret for ReflectionFactory to access non-public members in java.lang.reflect.

...

I can try moving the clinit code from Modifier to AccessibleObject, but I question is there some reason it is there? Would we be sure in moving it we are not missing something?

ReflectionFactory is the internal support for reflection. The methods that access LangReflectAccess shared secrets should have a Method, Field or Constructor in hand.  The ReflectionFactory::newField and newMethod that don't take Field/Method parameter are unused (I suspect they were used by the VM native reflection implementation previously. That led me to suggest to move setLangReflectAccess to AccessibleObject.

AccessibleObject is initialized very early during startup by the VM.  My proposed fix would change the list of classes loaded during early startup but it would need to look at closely.

Having a second thought, my proposed fix can be a follow-on clean up. I'm okay with your point fix that resolves JDK-8219378. I will file a JBS issue for the follow-on clean up.  What do you think?

thanks Mandy

Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Thanks for merging Roger Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leonard@uk.ibm.com From: Roger Riggs <Roger.Riggs@oracle.com> To: Andrew Leonard <andrew_m_leonard@uk.ibm.com>, Mandy Chung <mandy.chung@oracle.com> Cc: core-libs-dev@openjdk.java.net Date: 26/02/2019 17:00 Subject: Re: Fix proposal: JDK-8219378 NPE in ReflectionFactory.newMethodAccessor when langReflectAccess not initialized Hi Andrew, I've got the changeset you sent earlier and have tested it as well. I'll push it today unless there any objections. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-reflectionfactory-8219378-2/ Roger On 02/26/2019 11:46 AM, Andrew Leonard wrote: Hi Mandy, I think your last proposal sounds a good plan, push 8219378 to fix the immediate issue and create a new change to refactor the clinit location... As I am not a "Committer" (yet! working on it!) I can't use the Submit repo. I have built your patch and run a bunch of tests locally and it works great. I have also tested it successfully with J9. So I am happy with your change. If Roger & yourself are happy with 8219378, can we Submit-repo it for final test and get that one merged? Many thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leonard@uk.ibm.com From: Mandy Chung <mandy.chung@oracle.com> To: Andrew Leonard <andrew_m_leonard@uk.ibm.com> Cc: core-libs-dev@openjdk.java.net, Roger Riggs <Roger.Riggs@oracle.com> Date: 26/02/2019 00:16 Subject: Re: Fix proposal: JDK-8219378 NPE in ReflectionFactory.newMethodAccessor when langReflectAccess not initialized On 2/25/19 5:12 AM, Andrew Leonard wrote:

Hi Mandy, I must admit I don't completely follow the logic of the existing Modifier init of langReflectAccess, the comment indicates a "protocol between java.lang and java.lang.reflect".

That sets up the shared secret for ReflectionFactory to access non-public members in java.lang.reflect.

I can try moving the clinit code from Modifier to AccessibleObject, but I question is there some reason it is there? Would we be sure in moving it we are not missing something?

ReflectionFactory is the internal support for reflection. The methods that access LangReflectAccess shared secrets should have a Method, Field or Constructor in hand. The ReflectionFactory::newField and newMethod that don't take Field/Method parameter are unused (I suspect they were used by the VM native reflection implementation previously. That led me to suggest to move setLangReflectAccess to AccessibleObject. AccessibleObject is initialized very early during startup by the VM. My proposed fix would change the list of classes loaded during early startup but it would need to look at closely. Having a second thought, my proposed fix can be a follow-on clean up. I'm okay with your point fix that resolves JDK-8219378. I will file a JBS issue for the follow-on clean up. What do you think? thanks Mandy Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Hi Mandy, I can confirm "just" moving the clinit code into AccessibleObject does resolve the J9 issue as well, without changing ReflectionFactory... What's your thoughts? Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leonard@uk.ibm.com From: Mandy Chung <mandy.chung@oracle.com> To: Andrew Leonard <andrew_m_leonard@uk.ibm.com>, Roger Riggs <Roger.Riggs@oracle.com> Cc: core-libs-dev@openjdk.java.net Date: 22/02/2019 18:17 Subject: Re: Fix proposal: JDK-8219378 NPE in ReflectionFactory.newMethodAccessor when langReflectAccess not initialized Hi Andrew, Thanks for the stack trace of the issue triggering this. It seems to me that Modifier::<clinit> isn't the right place to setLangReflectAccess shared secret. It might have assumed that Modifier should have been initialized when Field/Method or other AccessibleObject is instantiated which isn't true since the modifiers field is an int. While the fix looks okay, would you mind trying a different fix moving Modifier::<clinit> to AccessibleObject? See this resolves the issue when running on J9? Mandy On 2/22/19 12:49 AM, Andrew Leonard wrote:

Hi Roger, I had a think about this and a testcase will be difficult, as it was found during OpenJ9 testing and occured during VM bootstrap, feel free to read further details here:

https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_eclipse_open...

So the issue was discovered due to the bootstrap behaviour, it was not observed with Hotspot, however given the obvious missing initialization check logic in the class it's not to say there's an untested route with Hotspot that could hit it... I'm not sure how I could scaffold a jtreg test to replicate the same? Thanks Andrew

Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU