jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

Wed Jun 5 03:31:08 PDT 2013

On Wed 05 Jun 2013 11:34:05 AM CEST, shanliang wrote:
> Jaroslav Bachorik wrote:
>> On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote:
>>
>>> On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote:
>>>
>>>> On 5/29/13 7:17 PM, Jaroslav Bachorik wrote:
>>>>
>>>>> On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote:
>>>>>
>>>>>> I would recommend against changing the code to do additional calls to
>>>>>> Class.forName during MBean introspection. As I recall we made the
>>>>>> opposite change some years ago, both because Class.forName can be slow
>>>>>> (it may call out to a user ClassLoader) and because it is a potential
>>>>>> source of security problems.
>>>>>>
>>>>> Thanks. I was trying to dig some history from mercurial but couldn't.
>>>>> Walking through all the related interfaces is equally acceptable - I've
>>>>> tried both of the solutions and they test well with the regtests.
>>>>>
>>>>> I am still puzzled by the current implementation which will fail to
>>>>> locate the correct MBean interface in eg.
>>>>>
>>>>> <<CInterface>> extends <<BInterface>> extends <<ServiceMBean>>
>>>>>
>>>>> ClassA extends Service implements <<CInterface>>
>>>>>
>>>>> as the process would stop on <<BInterface>> (checks the superclass of
>>>>> the ClassA, checks all the interfaces implemented by the Service class,
>>>>> checks all the interfaces extended by <<CInterface>>) which plainly
>>>>> does not conform to the MBean interface naming convention and would
>>>>> miss the <<ServiceMBean>> interface.
>>>>>
>>>> Hi Jaroslav,
>>>>
>>>> <<Service>> would have to implement <<ServiceMBean>> either
>>>> directly or indirectly.
>>>>
>>>> So the current implementation is correct.
>>>>
>>>> If <<ServiceMBean>> is not assignable from <<Service>> then
>>>> <<ServiceMBean>> is not an MBean interface for ClassA.
>>>>
>>> Actually, when you do
>>> ClassA extends Service implements <<BInterface>>
>>>
>>> the Introspector will return <<ServiceMBean>> as the standard mbean
>>> interface for ClassA. I've just tried it on a simple project to make
>>> sure I understand the code correctly. The puzzle is which behaviour is
>>> correct? Either all the levels of the interface hierarchy should be
>>> checked for the [className]MBean interfaces or none, I guess. However,
>>> I can not find anything in the spec related to this case.
>>>
>>
>> I've updated the webrev not to use Class.forName() when inferring the
>> MBean interface to use.
>>
>> However, I've changed the way the interface hierarchy is treated -
>> originally, all the interfaces of a certain class and their direct super
>> interfaces were checked for the MBean interface candidates (with
>> [class.Name]MBean class name) - but only one level of the hierarchy was
>> checked. The new implementation will check only the direct interfaces.
>>
>> I am still not sure whether the interface hierarchy should be searched
>> for the appropriately named interfaces or not. The spec does not say
>> anything about this and all the jtreg and jck tests are indifferent to
>> my changes. But I am sure that if it is supposed to search the interface
>> hierarchy it should not stop after the first level has been checked, as
>> is done by the current implementation.
>>
> Your modification changes the current JMX behavior, if the spec does
> not say about interface searching, I think it is better to keep the
> current behavior for the implementation compatibility.
>
> Here are examples:
> 1)    A extends B implements AMBean, BMBean
> the current JMX will get AMBean for the class A, but BMBean will be
> got with your modification.

No, it will return AMBean. Firstly, the A is interrogated and if there 
is an AMBean interface in the list of implemented interfaces it is 
returned.

> 2)    A extends B implements AMBean
> the current JMX will get AMBean for the class A, but which mbean
> interface will be found with your modification? I think it will be null.

No, it will return AMBean.

The only thing I've changed is that if you have BMBean extends AMBean 
and A implements BMBean the AMBean won't be returned. While the current 
implementation would return AMBean if we add CMBean extends BMBean and 
then A implements CMBean it will fail to return the AMBean even though 
it is virtually the same situation. IMO, the implementation should be 
consistent whether it walks up the interface hierarchy to find the 
MBean interface or not - and not to choose an arbitrary number of 
levels that will be checked (currently 2).

>
> 367             if ((Modifier.isPublic(mbeanInterface.getModifiers()) ||
>  368                  MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) &&
>  369                  clMBeanName.equals(mbeanInterface.getName())) {
>  370                 return mbeanInterface;
>  371             }
> As I indicated in my previous mail, better to write as:
>     if (clMBeanName.equals(mbeanInterface.getName() &&
>          (Modifier.isPublic(mbeanInterface.getModifiers()) ||
> MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN)) {
>
> checking name first because it must be cheaper here.

I am not sure - clMBeanName.equals(mbeanInterface.getName()) involves 
character by character comparison. MBeanAnalyzer.ALLOW_NON_PUBLIC_MBEAN 
is simple static field access and if mbeanInterface.getModifiers() 
isn't horribly slow we should be better off with my version. I can 
profile the code to find out the difference.

-JB-

>
> Shanliang
>
>> Webrev: http://cr.openjdk.java.net/~jbachorik/8010285/webrev.02
>>
>> -JB-
>>
>>
>>> -JB-
>>>
>>>
>>>> You can work around that by wrapping an instance of ClassA
>>>> in an instance of  javax.management.StandardMBean, and by
>>>> specifying <<ServiceMBean>>.class as the MBean interface
>>>> in the constructor.
>>>>
>>>> Hope this helps,
>>>>
>>>> -- daniel
>>>>
>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>>> Éamonn
>>>>>>
>>>>>>
>>>>>> 2013/5/29 Jaroslav Bachorik <jaroslav.bachorik at oracle.com
>>>>>> <mailto:jaroslav.bachorik at oracle.com>>
>>>>>>
>>>>>>      Updated webrev -
>>>>>>      http://cr.openjdk.java.net/~jbachorik/8010285/webrev.01
>>>>>>
>>>>>>      It adds regtests and takes care of the comments from David and
>>>>>>      Shanliang.
>>>>>>
>>>>>>      -JB-
>>>>>>
>>>>>>      On 05/28/2013 04:22 PM, Jaroslav Bachorik wrote:
>>>>>>      > The fix enforces the management interfaces (read MBean and
>>>>>> MXBean
>>>>>>      > interfaces) being public. While this is defined in the
>>>>>>      specification it
>>>>>>      > was not enforced in any way and it was allowed to create MBeans
>>>>>>      for eg.
>>>>>>      > private MBean interfaces.
>>>>>>      >
>>>>>>      > The fix adds checks when creating and registering MBeans and
>>>>>> throws
>>>>>>      > javax.management.NotCompliantMBeanException when a user tries to
>>>>>>      create
>>>>>>      > an MBean with non-public management interface.
>>>>>>      >
>>>>>>      > Since this change can cause problems for users having non-public
>>>>>>      > management interfaces a system property is introduced that will
>>>>>>      revert
>>>>>>      > to the old behaviour when set
>>>>>> (com.sun.jmx.mbeans.allowNonPublic).
>>>>>>      >
>>>>>>      > Thanks,
>>>>>>      >
>>>>>>      > -JB-
>>>>>>      >
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>
>>
>