From 27a969b2dd08a499d41aeab4a333c4b2c7bc996b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 20 Dec 2023 15:59:05 +0000 Subject: [PATCH 1/7] build(deps): bump com.github.spotbugs:spotbugs-maven-plugin Bumps [com.github.spotbugs:spotbugs-maven-plugin](https://github.com/spotbugs/spotbugs-maven-plugin) from 4.7.3.6 to 4.8.2.0. - [Release notes](https://github.com/spotbugs/spotbugs-maven-plugin/releases) - [Commits](https://github.com/spotbugs/spotbugs-maven-plugin/compare/spotbugs-maven-plugin-4.7.3.6...spotbugs-maven-plugin-4.8.2.0) --- updated-dependencies: - dependency-name: com.github.spotbugs:spotbugs-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 7d08ff02..09acb919 100644 --- a/pom.xml +++ b/pom.xml @@ -47,7 +47,7 @@ 1.16.1 4.8.3 - 4.7.3.6 + 4.8.2.0 5.10.1 2.2 5.2.0 From 94d2b44fa35210371e37539901bcf2eb2306ff0f Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 20 Dec 2023 11:09:43 -0500 Subject: [PATCH 2/7] clean up constructor throws --- .../io/cryostat/core/EventOptionsBuilder.java | 53 +++++++------------ .../core/net/JmxFlightRecorderService.java | 8 +-- 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/src/main/java/io/cryostat/core/EventOptionsBuilder.java b/src/main/java/io/cryostat/core/EventOptionsBuilder.java index 23f63fe6..7318c788 100644 --- a/src/main/java/io/cryostat/core/EventOptionsBuilder.java +++ b/src/main/java/io/cryostat/core/EventOptionsBuilder.java @@ -16,9 +16,9 @@ package io.cryostat.core; import java.io.IOException; +import java.util.Collection; import java.util.HashMap; import java.util.Map; -import java.util.function.Supplier; import org.openjdk.jmc.common.unit.IConstrainedMap; import org.openjdk.jmc.common.unit.IConstraint; @@ -27,36 +27,26 @@ import org.openjdk.jmc.common.unit.QuantityConversionException; import org.openjdk.jmc.flightrecorder.configuration.events.EventOptionID; import org.openjdk.jmc.flightrecorder.configuration.events.IEventTypeID; -import org.openjdk.jmc.rjmx.IConnectionHandle; import org.openjdk.jmc.rjmx.ServiceNotAvailableException; import org.openjdk.jmc.rjmx.services.jfr.IEventTypeInfo; import org.openjdk.jmc.rjmx.services.jfr.internal.FlightRecorderServiceV2; import io.cryostat.core.net.JFRConnection; -import io.cryostat.core.tui.ClientWriter; - -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; public class EventOptionsBuilder { - private final boolean isV2; private final IMutableConstrainedMap map; - private Map>> knownTypes; - private Map eventIds; - - public EventOptionsBuilder(ClientWriter cw, JFRConnection connection, Supplier v2) - throws ServiceNotAvailableException, IOException, - org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException { - this.isV2 = v2.get(); - this.map = connection.getService().getDefaultEventOptions().emptyWithSameConstraints(); - knownTypes = new HashMap<>(); - eventIds = new HashMap<>(); + private final Map>> knownTypes; + private final Map eventIds; - if (!isV2) { - cw.println("Flight Recorder V1 is not supported"); - } + public EventOptionsBuilder( + IMutableConstrainedMap empty, + Collection eventTypes) { + this.map = empty.emptyWithSameConstraints(); + this.knownTypes = new HashMap<>(); + this.eventIds = new HashMap<>(); - for (IEventTypeInfo eventTypeInfo : connection.getService().getAvailableEventTypes()) { + for (IEventTypeInfo eventTypeInfo : eventTypes) { eventIds.put( eventTypeInfo.getEventTypeID().getFullKey(), eventTypeInfo.getEventTypeID()); knownTypes.putIfAbsent( @@ -87,12 +77,8 @@ static V capture(T t) { return (V) t; } - @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Field is never mutated") public IConstrainedMap build() { - if (!isV2) { - return null; - } - return map; + return map.mutableCopy(); } public static class EventTypeException extends Exception { @@ -108,18 +94,17 @@ public static class EventOptionException extends Exception { } public static class Factory { - private final ClientWriter cw; - - public Factory(ClientWriter cw) { - this.cw = cw; - } - public EventOptionsBuilder create(JFRConnection connection) throws IOException, ServiceNotAvailableException, org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException { - IConnectionHandle handle = connection.getHandle(); - return new EventOptionsBuilder( - cw, connection, () -> FlightRecorderServiceV2.isAvailable(handle)); + if (!FlightRecorderServiceV2.isAvailable(connection.getHandle())) { + throw new UnsupportedOperationException("Only FlightRecorder V2 is supported"); + } + IMutableConstrainedMap empty = + connection.getService().getDefaultEventOptions().emptyWithSameConstraints(); + Collection eventTypes = + connection.getService().getAvailableEventTypes(); + return new EventOptionsBuilder(empty, eventTypes); } } } diff --git a/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java b/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java index a0af26fb..0123f110 100644 --- a/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java +++ b/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java @@ -30,14 +30,12 @@ import org.openjdk.jmc.flightrecorder.configuration.events.EventOptionID; import org.openjdk.jmc.flightrecorder.configuration.events.IEventTypeID; import org.openjdk.jmc.rjmx.ConnectionException; -import org.openjdk.jmc.rjmx.IConnectionHandle; import org.openjdk.jmc.rjmx.ServiceNotAvailableException; import org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException; import org.openjdk.jmc.rjmx.services.jfr.IEventTypeInfo; import org.openjdk.jmc.rjmx.services.jfr.IFlightRecorderService; import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor; import org.openjdk.jmc.rjmx.services.jfr.internal.FlightRecorderServiceFactory; -import org.openjdk.jmc.rjmx.services.jfr.internal.FlightRecorderServiceV2; import io.cryostat.core.EventOptionsBuilder; import io.cryostat.core.EventOptionsBuilder.EventOptionException; @@ -237,11 +235,7 @@ private IConstrainedMap enableAllEvents() throws ConnectionException, IOException, FlightRecorderException, ServiceNotAvailableException, QuantityConversionException, EventOptionException, EventTypeException { - - IConnectionHandle handle = conn.getHandle(); - EventOptionsBuilder builder = - new EventOptionsBuilder( - cw, conn, () -> FlightRecorderServiceV2.isAvailable(handle)); + EventOptionsBuilder builder = new EventOptionsBuilder.Factory().create(conn); for (IEventTypeInfo eventTypeInfo : conn.getService().getAvailableEventTypes()) { builder.addEvent(eventTypeInfo.getEventTypeID().getFullKey(), "enabled", "true"); From 060c0d34e3adf210bf9abfb989b9ec9045571036 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 20 Dec 2023 11:10:10 -0500 Subject: [PATCH 3/7] remove 'throws' declaration that never happens --- src/main/java/io/cryostat/core/agent/AgentJMXHelper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/io/cryostat/core/agent/AgentJMXHelper.java b/src/main/java/io/cryostat/core/agent/AgentJMXHelper.java index 4069418d..b3c78ed3 100644 --- a/src/main/java/io/cryostat/core/agent/AgentJMXHelper.java +++ b/src/main/java/io/cryostat/core/agent/AgentJMXHelper.java @@ -26,7 +26,6 @@ import javax.management.ObjectName; import javax.management.ReflectionException; -import org.openjdk.jmc.rjmx.ConnectionException; import org.openjdk.jmc.rjmx.IConnectionHandle; public class AgentJMXHelper { @@ -41,7 +40,7 @@ public class AgentJMXHelper { private final IConnectionHandle connectionHandle; private final MBeanServerConnection mbsc; - public AgentJMXHelper(IConnectionHandle connectionHandle) throws ConnectionException { + public AgentJMXHelper(IConnectionHandle connectionHandle) { this.connectionHandle = connectionHandle; mbsc = connectionHandle.getServiceOrDummy(MBeanServerConnection.class); } From e6e3cdbfd9c4f81e1f023186d335c23aebbcd058 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 20 Dec 2023 11:14:00 -0500 Subject: [PATCH 4/7] mitigate potential finalizer attack --- .../java/io/cryostat/core/agent/LocalProbeTemplateService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/io/cryostat/core/agent/LocalProbeTemplateService.java b/src/main/java/io/cryostat/core/agent/LocalProbeTemplateService.java index 1f3a9677..99161674 100644 --- a/src/main/java/io/cryostat/core/agent/LocalProbeTemplateService.java +++ b/src/main/java/io/cryostat/core/agent/LocalProbeTemplateService.java @@ -65,6 +65,8 @@ public LocalProbeTemplateService(FileSystem fs, Environment env) throws IOExcept } } + protected final void finalize() {} + public ProbeTemplate addTemplate(InputStream inputStream, String filename) throws FileAlreadyExistsException, IOException, SAXException { Path path = fs.pathOf(env.getEnv(TEMPLATE_PATH), filename); From 8338a643a3fda9da147e229b6c4dd06ef1a4ccbf Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 20 Dec 2023 11:14:10 -0500 Subject: [PATCH 5/7] remove 'throws' declarations that never happen --- src/main/java/io/cryostat/core/net/JFRJMXConnection.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/io/cryostat/core/net/JFRJMXConnection.java b/src/main/java/io/cryostat/core/net/JFRJMXConnection.java index 18d0d831..f734796e 100644 --- a/src/main/java/io/cryostat/core/net/JFRJMXConnection.java +++ b/src/main/java/io/cryostat/core/net/JFRJMXConnection.java @@ -77,8 +77,7 @@ public class JFRJMXConnection implements JFRConnection { FileSystem fs, Environment env, IConnectionDescriptor cd, - List listeners) - throws ConnectionException { + List listeners) { this.cw = cw; this.fs = fs; this.env = env; @@ -87,8 +86,7 @@ public class JFRJMXConnection implements JFRConnection { this.serviceFactory = new FlightRecorderServiceFactory(); } - JFRJMXConnection(ClientWriter cw, FileSystem fs, Environment env, IConnectionDescriptor cd) - throws ConnectionException { + JFRJMXConnection(ClientWriter cw, FileSystem fs, Environment env, IConnectionDescriptor cd) { this(cw, fs, env, cd, List.of()); } From ffb1bafa55e9881ffc5ebef53d9437f0b9abdfc7 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 20 Dec 2023 11:26:11 -0500 Subject: [PATCH 6/7] mitigate incomplete construction finalizer problem --- .../core/serialization/SerializableRecordingDescriptor.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/io/cryostat/core/serialization/SerializableRecordingDescriptor.java b/src/main/java/io/cryostat/core/serialization/SerializableRecordingDescriptor.java index 74eabcdc..1d89819a 100644 --- a/src/main/java/io/cryostat/core/serialization/SerializableRecordingDescriptor.java +++ b/src/main/java/io/cryostat/core/serialization/SerializableRecordingDescriptor.java @@ -101,6 +101,8 @@ public SerializableRecordingDescriptor(SerializableRecordingDescriptor o) { this.maxAge = o.getMaxAge(); } + protected final void finalize() {} + /** * @see {@link org.openjdk.jmc.rjmx.services.jfr.internal.RecordingDescriptorV2#decideState} */ From d03ea2f0284e6444f045c2f6ab198025aefbc43e Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 20 Dec 2023 11:26:33 -0500 Subject: [PATCH 7/7] delay connection attempt and throws until call time --- .../cryostat/core/net/JFRJMXConnection.java | 2 +- .../core/net/JmxFlightRecorderService.java | 107 +++++++++++------- 2 files changed, 67 insertions(+), 42 deletions(-) diff --git a/src/main/java/io/cryostat/core/net/JFRJMXConnection.java b/src/main/java/io/cryostat/core/net/JFRJMXConnection.java index f734796e..f1244e25 100644 --- a/src/main/java/io/cryostat/core/net/JFRJMXConnection.java +++ b/src/main/java/io/cryostat/core/net/JFRJMXConnection.java @@ -106,7 +106,7 @@ public synchronized IConnectionHandle getHandle() throws ConnectionException, IO public synchronized CryostatFlightRecorderService getService() throws ConnectionException, IOException, ServiceNotAvailableException { - return new JmxFlightRecorderService(this, cw); + return new JmxFlightRecorderService(this); } public TemplateService getTemplateService() { diff --git a/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java b/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java index 0123f110..15c08dfe 100644 --- a/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java +++ b/src/main/java/io/cryostat/core/net/JmxFlightRecorderService.java @@ -42,105 +42,111 @@ import io.cryostat.core.EventOptionsBuilder.EventTypeException; import io.cryostat.core.templates.Template; import io.cryostat.core.templates.TemplateType; -import io.cryostat.core.tui.ClientWriter; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class JmxFlightRecorderService implements CryostatFlightRecorderService { + private final Logger logger = LoggerFactory.getLogger(getClass()); private final JFRJMXConnection conn; - private final IFlightRecorderService delegate; - private final ClientWriter cw; - JmxFlightRecorderService(JFRJMXConnection conn, ClientWriter cw) - throws ConnectionException, ServiceNotAvailableException, IOException { + JmxFlightRecorderService(JFRJMXConnection conn) { this.conn = conn; - if (!conn.isConnected()) { - conn.connect(); - } - IFlightRecorderService service = - new FlightRecorderServiceFactory().getServiceInstance(conn.getHandle()); - if (service == null || !conn.isConnected()) { - throw new ConnectionException( - String.format( - "Could not connect to remote target %s", - conn.connectionDescriptor.createJMXServiceURL().toString())); + } + + protected IFlightRecorderService tryConnect() throws FlightRecorderException { + try { + if (!conn.isConnected()) { + conn.connect(); + } + IFlightRecorderService service = + new FlightRecorderServiceFactory().getServiceInstance(conn.getHandle()); + if (service == null || !conn.isConnected()) { + throw new ConnectionException( + String.format( + "Could not connect to remote target %s", + conn.connectionDescriptor.createJMXServiceURL().toString())); + } + return service; + } catch (IOException | ServiceNotAvailableException e) { + throw new FlightRecorderException("Connection failed", e); } - this.delegate = service; - this.cw = cw; } @Override public List getAvailableRecordings() throws FlightRecorderException { - return delegate.getAvailableRecordings(); + return tryConnect().getAvailableRecordings(); } @Override public IRecordingDescriptor getSnapshotRecording() throws FlightRecorderException { - return delegate.getSnapshotRecording(); + return tryConnect().getSnapshotRecording(); } @Override public IRecordingDescriptor getUpdatedRecordingDescription(IRecordingDescriptor descriptor) throws FlightRecorderException { - return delegate.getUpdatedRecordingDescription(descriptor); + return tryConnect().getUpdatedRecordingDescription(descriptor); } @Override public IRecordingDescriptor start( IConstrainedMap recordingOptions, IConstrainedMap eventOptions) throws FlightRecorderException { - return delegate.start(recordingOptions, eventOptions); + return tryConnect().start(recordingOptions, eventOptions); } @Override public void stop(IRecordingDescriptor descriptor) throws FlightRecorderException { - delegate.stop(descriptor); + tryConnect().stop(descriptor); } @Override public void close(IRecordingDescriptor descriptor) throws FlightRecorderException { - delegate.close(descriptor); + tryConnect().close(descriptor); } @Override public Map> getAvailableRecordingOptions() throws FlightRecorderException { - return delegate.getAvailableRecordingOptions(); + return tryConnect().getAvailableRecordingOptions(); } @Override public IConstrainedMap getRecordingOptions(IRecordingDescriptor recording) throws FlightRecorderException { - return delegate.getRecordingOptions(recording); + return tryConnect().getRecordingOptions(recording); } @Override public Collection getAvailableEventTypes() throws FlightRecorderException { - return delegate.getAvailableEventTypes(); + return tryConnect().getAvailableEventTypes(); } @Override public Map getEventTypeInfoMapByID() throws FlightRecorderException { - return delegate.getEventTypeInfoMapByID(); + return tryConnect().getEventTypeInfoMapByID(); } @Override public IConstrainedMap getCurrentEventTypeSettings() throws FlightRecorderException { - return delegate.getCurrentEventTypeSettings(); + return tryConnect().getCurrentEventTypeSettings(); } @Override public IConstrainedMap getEventSettings(IRecordingDescriptor recording) throws FlightRecorderException { - return delegate.getEventSettings(recording); + return tryConnect().getEventSettings(recording); } @Override public InputStream openStream(IRecordingDescriptor descriptor, boolean removeOnClose) throws FlightRecorderException { - return delegate.openStream(descriptor, removeOnClose); + return tryConnect().openStream(descriptor, removeOnClose); } @Override @@ -150,58 +156,76 @@ public InputStream openStream( IQuantity endTime, boolean removeOnClose) throws FlightRecorderException { - return delegate.openStream(descriptor, startTime, endTime, removeOnClose); + return tryConnect().openStream(descriptor, startTime, endTime, removeOnClose); } @Override public InputStream openStream( IRecordingDescriptor descriptor, IQuantity lastPartDuration, boolean removeOnClose) throws FlightRecorderException { - return delegate.openStream(descriptor, lastPartDuration, removeOnClose); + return tryConnect().openStream(descriptor, lastPartDuration, removeOnClose); } @Override public List getServerTemplates() throws FlightRecorderException { - return delegate.getServerTemplates(); + return tryConnect().getServerTemplates(); } @Override public void updateEventOptions( IRecordingDescriptor descriptor, IConstrainedMap options) throws FlightRecorderException { - delegate.updateEventOptions(descriptor, options); + tryConnect().updateEventOptions(descriptor, options); } @Override public void updateRecordingOptions( IRecordingDescriptor descriptor, IConstrainedMap options) throws FlightRecorderException { - delegate.updateRecordingOptions(descriptor, options); + tryConnect().updateRecordingOptions(descriptor, options); } @Override public boolean isEnabled() { - return delegate.isEnabled(); + try { + return tryConnect().isEnabled(); + } catch (FlightRecorderException e) { + logger.error("Connection failed", e); + return false; + } } @Override public void enable() throws FlightRecorderException { - delegate.enable(); + tryConnect().enable(); } @Override public String getVersion() { - return delegate.getVersion(); + try { + return tryConnect().getVersion(); + } catch (FlightRecorderException e) { + logger.error("Connection failed", e); + return "unknown"; + } } @Override public IDescribedMap getDefaultRecordingOptions() { - return delegate.getDefaultRecordingOptions(); + try { + return tryConnect().getDefaultRecordingOptions(); + } catch (FlightRecorderException e) { + throw new RuntimeException(e); + } } @Override public IDescribedMap getDefaultEventOptions() { - return delegate.getDefaultEventOptions(); + try { + return tryConnect().getDefaultEventOptions(); + } catch (FlightRecorderException e) { + throw new RuntimeException(e); + } } @Override @@ -213,7 +237,8 @@ public IRecordingDescriptor start( ConnectionException, IOException, FlightRecorderException, ServiceNotAvailableException, QuantityConversionException, EventOptionException, EventTypeException { - return delegate.start(recordingOptions, enableEvents(templateName, preferredTemplateType)); + return tryConnect() + .start(recordingOptions, enableEvents(templateName, preferredTemplateType)); } private IConstrainedMap enableEvents(