RFR: 7150256: Add back Diagnostic Command JMX API

[Frederic Parain]

Thank you for this feedback, my comments are in-lined below.

The updated webrev is here:
http://cr.openjdk.java.net/~fparain/7150256/webrev.01/

On 12/17/12 02:12 PM, Jaroslav Bachorik wrote:
> Hi Frederic,
>
> I've found the following issues when inspecting the changes in the Java
> code:
>
> JB1: src/share/classes/sun/management/VMManagementImpl.java
> getDiagnosticCommands(), getDiagnosticCommandInfo() and
> executeDiagnosticCommand() are all publicly accessible native methods
> accepting the user input. According to the secure coding guide the
> native methods should be wrapped in public java methods performing the
> necessary user input validation.

This is a valid point according to the secure coding guidelines,
however, there's some aspects to consider here:

   1) a Java application cannot access directly to a VMManagementImpl
      instance. The class is package protected and no reference is leaked
      through the public management APIs.

   2) With Diagnostic Commands, there's no input validation that could
      be performed at the Java level, because there's no way for the
      DiagnosticCommandsMBean to know the syntax of the different
      commands (it doesn't even know the list of existing commands).
      By design, each DiagnosticCommand can have its own syntax.
      The only input validation actually performed is in the Wrapper
      (DiagnosticCommandImpl$Wrapper) where a null element in a
      Sting array causes an exception to be thrown.

   3) The DiagnosticCommandMBean is not the only way to invoke
      Diagnostic Command. They can also be invoked with the jcmd
      tool using the attachAPI. So, inside the VM, there's already
      the logic to parse and validate arguments. Trying to add
      input validation in VMManagementImpl would cause code duplication
      and would require a more complex API to export DCMD syntax from
      the JVM to the Java level.

> JB2: src/share/classes/sun/management/DiagnosticCommandArgumentInfo.java
> The file is new but has "@since 7u6" (should be @since 7u12)

This class is not public anymore, but I'll update the tag.

> JB3: src/share/classes/sun/management/DiagnosticCommandImpl.java
> In method getMBeanInfo() the wrapper map is first created (149-159) and
> immediately iterated over (164-182). It seems possible to merge those
> two passes and save some CPU.

Done.

> JB4: src/share/classes/sun/management/DiagnosticCommandImpl.java
> Line 249 - typo "dcmd.permissionCame" instead of "dcmd.permissionName"

Fixed.

Thank you for the review,

Fred

> On 12/17/2012 12:27 PM, Frederic Parain wrote:
>> Hi,
>>
>> Please review the following change for CR 7150256.
>>
>> This changeset is the JDK side of two parts project.
>>
>> The goal of this feature is to allow remote invocations of
>> Diagnostic Commands via JMX.
>>
>> Diagnostic Commands are actually invoked from the jcmd
>> tool, which is a local tool using the attachAPI. It was
>> not possible to remotely invoke these commands.
>> This changeset creates a new PlatformMBean, remotely
>> accessible via the Platform MBean Server, providing
>> access to Diagnostic Commands too.
>>
>> The set of supported Diagnostic Commands varies with
>> the JVM version, command line options and even some
>> runtime operations. The new PlatformMBean, called
>> DiagnosticCommandsMBean is not statically defined,
>> but computes its content dynamically at runtime.
>> This is why the MBeanInfo returned by this new
>> MBean contains a lot of meta-data describing
>> each supported Diagnostic Command (name, help
>> message, syntax, etc.)
>>
>> Because this feature allows remote invocations, it
>> is important to be able to control who is invoking
>> and what is invoked. Java Permission checks have
>> been introduced before a remote invocation request
>> is forwarded to the JVM. So, each Diagnostic Command
>> can require a Java Permission to be invoked remotely.
>> The name of the required Java Permission is provided
>> in the meta-data in the MBeanInfo.
>> The security configuration is done using a Security
>> Manager and the regular JMX configurations. Some
>> existing Diagnostic Commands have been extended to
>> specify which Java Permission they require.
>>
>> The webrev is here:
>>
>> http://cr.openjdk.java.net/~fparain/7150256/webrev.00/
>>
>> The second part of this project is the support for this
>> feature in the HotSpot VM, and is tracked by CR 8004095
>> (separate request for review will be sent for it).
>>
>> Thanks,
>>
>> Fred
>>
>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at Oracle.com