PING: RFR: 8225694: Destination option missing in FlightRecorderMXBeanImpl

[Chihiro Ito]

Thank you for reviewing.
Yes, please sponsor me.

Regards,
Chihiro

2019年10月7日(月) 20:50 Erik Gahlin <erik.gahlin at oracle.com>:

> Looks good.
>
> Do you want me to sponsor your fix?
>
> Thanks
> Erik
>
> Hi Erik,
>
> Thank you for reviewing again.
>
> I improved focusing on checking the destination and calling setter for
> destination.
> How about this fix?
>
> JBS https://bugs.openjdk.java.net/browse/JDK-8225694
> webrev http://cr.openjdk.java.net/~cito/JDK-8225694/webrev.02/
>
> This fix passed test-tier1 and test/jdk/jdk/jfr.
>
> Regards,
> Chihiro
>
>
> 2019年10月2日(水) 23:28 Erik Gahlin <erik.gahlin at oracle.com>:
>
>> Hi,
>>
>> I think it is fine to use Recording::setDestination(...)
>>
>> recording.setDestination(Paths.get(destination));
>>
>> It's only when the destination is read that the path object should not be
>> returned. I now see that getDestinationOriginalText() is already
>> implemented.
>>
>> The verification of the path is the tricky thing. Today nothing is being
>> done.
>>
>> Looking at the code, there doesn't seem to be a check in
>> PlatformingRecording/Recording::setDestination, even though the javadoc
>> states that "@throws IOException if the path is not writable". Not sure
>> why, but it is outside the scope of this bug.
>>
>> Maybe you could add a method checkDestination to PlatformRecording:
>>
>> public void setDestination(WriteableUserPath userSuppliedPath) throws
>> IOException {
>>   synchronized (recorder) {
>>     checkSetDestination(userSuppliedPath);
>>     this.destination = userSuppliedPath;
>>   }
>> }
>>
>> public void checkSetDestination(WriteableUserPath userSuppliedPath)
>> throws IOException {
>>   synchronized (recorder) {
>>     if (Utils.isState(getState(), RecordingState.STOPPED,
>> RecordingState.CLOSED)) {
>>       throw new IllegalStateException("Destination can't be set on a
>> recording that has been stopped/closed");
>>    }
>>   }
>> }
>>
>> and then call it over ManagementSupport::checkSetDestination(...) when
>> the option is validated.
>>
>> We can then later improve the validation and make sure it works for both
>> JMX and ordinary use of setDestination.
>>
>> Thanks
>> Erik
>>
>> Hi Erik,
>>
>> Could you review this again, please?
>>
>> Regards,
>> Chihiro
>>
>> 2019年9月26日(木) 0:57 Chihiro Ito <chiroito107 at gmail.com>:
>>
>>> Hi Erik,
>>>
>>> Thank you for reviewing and advising.
>>> I modified it. Could you review this, please?
>>>
>>> JBS https://bugs.openjdk.java.net/browse/JDK-8225694
>>> webrev http://cr.openjdk.java.net/~cito/JDK-8225694/webrev.01/
>>>
>>> Regards,
>>> Chihiro
>>>
>>> 2019年9月15日(日) 6:34 Erik Gahlin <erik.gahlin at oracle.com>:
>>>
>>>> Hi Chihiro.
>>>>
>>>> I noticed that you do.
>>>>
>>>> r.getDestination().toString();
>>>>
>>>> I’m not sure if it safe. A malicious user could implement their own
>>>> version of a Path object and have it execute in the context of the
>>>> MBeanServer thread. I would prefer if you would access the
>>>> PlatformRecording object (see PrivateAccess::getPlatformRecording) and use
>>>> WriteableUserPath#getText to get a textual representation of a path you can
>>>> return to getOptions(). You can add a method to the class
>>>> jdk.jfr.internal.managemnet.ManagementSupport, i.e
>>>> getRecordingDestintaion(Recording), where you can tunnel things to the
>>>> jdk.management.jfr module.
>>>>
>>>> Maybe you could also add the message “Not a valid destination” to the
>>>> IAE.
>>>>
>>>> Thanks
>>>> Erik
>>>>
>>>> > On 14 Sep 2019, at 19:06, Chihiro Ito < <chiroito107 at gmail.com>
>>>> chiroito107 at gmail.com> wrote:
>>>> >
>>>> > Hi,
>>>> >
>>>> > Could you review this tiny change, please?
>>>> >
>>>> > JBS https://bugs.openjdk.java.net/browse/JDK-8225694
>>>> > Webrev http://cr.openjdk.java.net/~cito/JDK-8225694/webrev.00/
>>>> >
>>>> > Regards,
>>>> > Chihiro
>>>>
>>>>
>>
>