PING: RFR: 8225694: Destination option missing in FlightRecorderMXBeanImpl

[Erik Gahlin]

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/ 
> <http://cr.openjdk.java.net/%7Ecito/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 
> <mailto: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
>>     <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
>>
>