jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public
shanliang
shanliang.jiang at oracle.com
Wed Jun 5 02:34:05 PDT 2013
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.
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.
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.
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-
>>>>> >
>>>>>
>>>>>
>>>>>
>>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130605/2beda5c1/attachment.html
More information about the serviceability-dev
mailing list