On 12/17/2012 03:58 PM, Frederic Parain wrote: > 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.
Ok. I see. If there is already a code checking for e.g. null arguments in the native part there is no need to duplicate it. I have no further comments for the java part of the patch. -JB- > >> 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 >>> >> >
