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-boun...@openjdk.java.net> on behalf of mandy chung <mandy.ch...@oracle.com>
*Sent:* Tuesday, October 24, 2017 7:20:27 PM
*To:* serviceability-dev@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-boun...@openjdk.java.net> on behalf of mandy chung <mandy.ch...@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
>



Reply via email to