PING: RFR: 8225694: Destination option missing in FlightRecorderMXBeanImpl

[Chihiro Ito]

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
>>>
>>>
>