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

[Jaroslav Bachorik]

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-

>
> 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-
>>>>
>>>>
>>>
>>>
>>
>