PING: RFR: 8225694: Destination option missing in FlightRecorderMXBeanImpl

[Erik Gahlin]

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 
> <mailto: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/
>     <http://cr.openjdk.java.net/%7Ecito/JDK-8225694/webrev.01/>
>
>     Regards,
>     Chihiro
>
>     2019年9月15日(日) 6:34 Erik Gahlin <erik.gahlin at oracle.com
>     <mailto: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
>         <mailto: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/
>         <http://cr.openjdk.java.net/%7Ecito/JDK-8225694/webrev.00/>
>         >
>         > Regards,
>         > Chihiro
>