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

[Daniel Fuchs]

On 5/29/13 4:44 PM, Jaroslav Bachorik wrote:
> On Wed 29 May 2013 03:38:59 PM CEST, Daniel Fuchs wrote:
>> On 5/29/13 1:18 PM, shanliang wrote:
>>> Jaroslav,
>>>
>>> Introspector.java
>>> ---------------------
>>> Line 496 - 515
>>> It is good to do check:
>>>       (Modifier.isPublic(c.getModifiers()) ||
>>> MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN)
>>> but it is not necessary if an interface is not equal to clMBeanName.
>>>
>>> is it possible to simplify the method as:
>>>        private static <T> Class<? super T> implementsMBean(Class<T> c,
>>> String clName) {
>>>           Class<T> ret = null;
>>>
>>>           try {
>>>               Class clMBeanClass = Class.forName(clName + "MBean");
>>
>> Hi Shanliang,
>>
>> I 'm not sure whether that would actually simplify anything.
>> Is attempting to load a class (which BTW would require to pass
>> the appropriate ClassLoader to Class.forName) faster or simpler
>> than using the current algorithm?
>
> Well, it seems to simplify the algorithm a bit. Currently, the run
> includes walking up the class hierarchy, retrieving all the interfaces
> for a class being inspected and the superinterfaces of the interfaces
> recursively, to check if any of them complies with the MBean interface
> naming. This can turn out to be a quite expensive process.
> Additionally, the current implementation seems to go through only 2
> levels of interfaces (the interfaces implemented by the inspected class
> and their direct super interfaces) and it can fail to resolve an MBean
> interface in some corner cases.
>
> On the other hand, when using the Class.forName() it is enough to walk
> up the super class hierarchy, try to load a <className>MBean class for
> each inspected class and determine whether such interface exists and is
> a valid MBean interface. I am not sure how expensive is trying to load
> a non-existent class but Class.getInterfaces() is a native method as
> well as Class.forName0() generating the same JNI overhead.
>
> -JB-

I would be wary of changing this code without extensive testing.
You will have to read the spec in details and make sure that you
haven't forgotten anything.

For a given implementation class <a.AImpl> there may be
several interfaces that match the MBean interface pattern - and
the spec clearly defines which should have precedence.
For instance if you have:

a.AImpl extends a.A extends b.BImpl extends b.B extends c.C
    implements a.AMBean {
}

with c.C implements c.CMBean, b.BMBean { }

then which is the MBean interface for a.AImpl?

I think it's b.BMBean - although I would have to read the spec twice
to be really sure ;-)

-- daniel



>
>>
>> But you're right - in isMBeanInterface the first check should
>> probably be c.getName().equals(clMBeanName).
>>
>> -- daniel
>>
>>>               List list = Arrays.asList(c.getInterfaces());
>>>
>>>               if (list.contains(clMBeanClass)
>>>                       && (Modifier.isPublic(clMBeanClass.getModifiers())
>>> || MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN)) {
>>>                   ret = clMBeanClass;
>>>               }
>>>           } catch (ClassNotFoundException cne) {
>>>               // clMBeanClass does not exist?
>>>           }
>>>
>>>           return ret;
>>>       }
>>>
>>> Is there any special reason to not have a unit test?
>>>
>>> Shanliang
>>>
>>>
>>> Jaroslav Bachorik wrote:
>>>> And the webrev would come handy, of course.
>>>>
>>>> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.00/
>>>>
>>>> -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-
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>