RFR : JDK-8044122 MBean access to the PID

[mandy chung]

On 10/26/17 1:53 AM, Bernd Eckenfels wrote:
> Hello,
>
> Yes the frames must have the security granted, isnt that the whole 
> purpose of a security permission check?
>
> With the current state of the patch unprivileged application code can 
> read the PID. If this is intentional I would simply remove the access 
> check completely instead.
>

Process::pid does not do any security check (see javadoc).  It's 
ProcessHandle::current that requires RuntimePermission("manageProcess") 
and getting it in a privileged context is correct and it's 
implementation detail.   We must ensure not leaking ProcessHandle.

Mandy

> Gruss
> Bernd
> -- 
> http://bernd.eckenfels.net
> ------------------------------------------------------------------------
> *From:* serviceability-dev 
> <serviceability-dev-bounces at openjdk.java.net> on behalf of mandy chung 
> <mandy.chung at oracle.com>
> *Sent:* Tuesday, October 24, 2017 7:20:27 PM
> *To:* serviceability-dev at openjdk.java.net
> *Subject:* Re: RFR : JDK-8044122 MBean access to the PID
> The permission check should be done separately, if needed. Otherwise, 
> the frames in the stack when this method is called must have the 
> security permission granted.
>
> Mandy
>
> On 10/23/17 1:00 PM, Bernd Eckenfels wrote:
>> Hello,
>>
>> When running this privileged it means one can bypass the permission 
>> by using the MBean, is that intentional? (Besides it is already 
>> available as the JMVID)
>>
>> Gruss
>> Bernd
>> -- 
>> http://bernd.eckenfels.net
>> ------------------------------------------------------------------------
>> *From:* serviceability-dev 
>> <serviceability-dev-bounces at openjdk.java.net> on behalf of mandy 
>> chung <mandy.chung at oracle.com>
>> *Sent:* Friday, October 20, 2017 4:42:00 PM
>> *To:* Ujwal Vangapally; Roger Riggs
>> *Cc:* serviceability-dev
>> *Subject:* Re: RFR : JDK-8044122 MBean access to the PID
>> Process::pid may throw SecurityException.  You have to wrap the call
>> with doPrivileged.   Process::pid can throw UOE on platform that doesn't
>> support this operation.  RuntimeMXBean::getPid should specify when the
>> platform does not support this operation.
>>
>> ProcessIdTest - can you also check if getName contains the pid matching
>> the value of getPid()?
>>
>> Mandy
>>
>> On 10/20/17 2:07 AM, Ujwal Vangapally wrote:
>> > kindly see the new webrev incorporating review comments.
>> >
>> > webrev:
>> > 
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.01/ 
>> <http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8044122/webrev.01/>
>> >
>> > Thanks,
>> >
>> > Ujwal.
>> >
>> >
>> > On 10/11/2017 3:50 PM, Ujwal Vangapally wrote:
>> >> Thanks for the review and suggestions Mandy, Roger.
>> >>
>> >> kindly see my comments inline.
>> >>
>> >>
>> >> On 10/10/2017 11:25 PM, Roger Riggs wrote:
>> >>> Hi Ujwal,
>> >>>
>> >>> In the implementation RuntimeMXBean.java: 72:  Include a message
>> >>> "getProcessId" in the throw new Unsupported...
>> >>> In the text and @return change "PID" to "process ID" as Alan 
>> suggested.
>> >>> 66: the @implSpec should be on its own line so the text starts on a
>> >>> new line to make the source more readable.
>> >>>
>> >>> Adding a test for getProcessId() should fit into one of the existing
>> >>> tests that spawns and then checks
>> >>> the attributes of a vm.  Perhaps MXBeanInteropTest1.java
>> >> I will make changes as suggested.
>> >>>
>> >>> Roger
>> >>>
>> >>>
>> >>>
>> >>> On 10/10/2017 1:20 PM, mandy chung wrote:
>> >>>>
>> >>>>
>> >>>> On 10/10/17 4:47 AM, Ujwal Vangapally wrote:
>> >>>>> Kindly review the changes made.
>> >>>>>
>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8044122
>> >>>>>
>> >>>>> webrev :
>> >>>>> 
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8044122/webrev.00/ 
>> <http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8044122/webrev.00/> 
>>
>> >>>>>
>> >>>>>
>> >>>>
>> >>>> RuntimeMXBean.java
>> >>>>    @since is missing
>> >>>>
>> >> I will add it.
>> >>>>    Process::pid is long rather than int. The javadoc for this
>> >>>> method should be consistent with Process::pid, as Alan points out.
>> >> will do it.
>> >>>>
>> >>>> VMManagementImpl.java
>> >>>>     I think getProcessId should probably be replaced to implement
>> >>>> with ProcessHandle.current().pid();
>> >>>>
>> >> you mean it would be better to use ProcessHandle.current().pid(); in
>> >> RuntimeImpl.java instead of jvm.getVmPid();
>> >>
>> >> kindly clarify.
>> >>>> Please include an unit test for it.
>> >>>>
>> >> will it be sufficient to add it to existing test 
>> MXBeanInteropTest1.java
>> >>
>> >>              System.out.println("getName\t\t"
>> >>                      + runtime.getName());
>> >> +            System.out.println("getPid\t\t"
>> >> +                    + runtime.getPid());
>> >> System.out.println("getSpecName\t\t"
>> >>                      + runtime.getSpecName());
>> >>
>> >>>> Mandy
>> >>>
>> >>
>> >> Ujwal
>> >
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171026/3276e1fd/attachment-0001.html>