From 9767073b3225af9736c5e528bb0eaa2fc930338e Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Fri, 25 Jul 2025 19:35:25 +0000 Subject: [PATCH 1/2] GP-5553: Disable timing out when we have a ProgressService. --- .../debug/gui/control/TargetActionTask.java | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/control/TargetActionTask.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/control/TargetActionTask.java index 24fda8c170..cb3a3b1f4b 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/control/TargetActionTask.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/control/TargetActionTask.java @@ -4,9 +4,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -136,6 +136,16 @@ public class TargetActionTask extends Task { /** * Execute an {@link ActionEntry} * + *

+ * If the {@link ProgressService} is available, we will not enforce a timeout, because it should + * be relatively easy for the user to manage the pending tasks. Otherwise, we'll enforce the + * timeout. The rationale here is that some tasks do actually take a good bit of time. For + * example, some targets just have a large module list. Often a GUI component is asking for a + * reason, and if we time it out, that thing doesn't get what it needs. Furthermore, the entry + * disappears from the task list, even though the back-end is likely still working on it. That's + * not good, actually. Since we have a cancel button, let the user decide when it's had enough + * time. + * * @param tool the tool in which to execute * @param title the title, often {@link ActionEntry#display()} * @param entry the action to execute @@ -144,11 +154,13 @@ public class TargetActionTask extends Task { */ public static CompletableFuture runAction(PluginTool tool, String title, ActionEntry entry) { - return executeTask(tool, new TargetActionTask(tool, title, entry)); + return executeTask(tool, new TargetActionTask(tool, title, entry, + tool.getService(ProgressService.class) == null)); } private final PluginTool tool; private final ActionEntry entry; + private final boolean timeout; /** * Construct a task fore the given action @@ -156,17 +168,24 @@ public class TargetActionTask extends Task { * @param tool the plugin tool * @param title the title, often {@link ActionEntry#display()} * @param entry the action to execute + * @param timeout whether or not to enforce the timeout */ - public TargetActionTask(PluginTool tool, String title, ActionEntry entry) { + public TargetActionTask(PluginTool tool, String title, ActionEntry entry, boolean timeout) { super(title, false, false, false); this.tool = tool; this.entry = entry; + this.timeout = timeout; } @Override public void run(TaskMonitor monitor) throws CancelledException { try { - entry.run(entry.requiresPrompt()); + if (timeout) { + entry.run(entry.requiresPrompt()); + } + else { + entry.invokeAsyncWithoutTimeout(entry.requiresPrompt()).get(); + } } catch (Throwable e) { reportError(e); From 1ad0a0f71917158cdc5a4bf199211f6481667e74 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Mon, 28 Jul 2025 12:31:48 +0000 Subject: [PATCH 2/2] GP-5788: Add an action to forcibly close all transactions for a target (in case of back-end misbehavior) --- .../java/ghidra/debug/api/target/Target.java | 11 ++++ .../api/tracermi/TraceRmiConnection.java | 13 ++++- .../TraceRmiConnectionManagerPlugin.html | 10 ++++ .../TraceRmiConnectionManagerProvider.java | 40 ++++++++++++++ .../service/tracermi/TraceRmiHandler.java | 54 ++++++++++++++----- .../service/tracermi/TraceRmiTarget.java | 5 ++ .../tracermi/TestTraceRmiConnection.java | 4 ++ .../plugin/core/debug/service/MockTarget.java | 4 ++ 8 files changed, 127 insertions(+), 14 deletions(-) diff --git a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/target/Target.java b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/target/Target.java index 4064db3b44..68473599f2 100644 --- a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/target/Target.java +++ b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/target/Target.java @@ -700,4 +700,15 @@ public interface Target { * @return true if busy */ boolean isBusy(); + + /** + * Forcibly commit all of the back-ends transactions on this target's trace. + * + *

+ * This is generally not a recommended course of action, except that sometimes the back-end + * crashes and fails to close a transaction. It should only be invoked by a relatively hidden + * menu option, and mediated by a warning of some sort. Closing a transaction prematurely, when + * the back-end actually does still need it may cause a host of other problems. + */ + void forciblyCloseTransactions(); } diff --git a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/tracermi/TraceRmiConnection.java b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/tracermi/TraceRmiConnection.java index 8e0ad284ff..9afc226bf0 100644 --- a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/tracermi/TraceRmiConnection.java +++ b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/tracermi/TraceRmiConnection.java @@ -171,8 +171,19 @@ public interface TraceRmiConnection extends AutoCloseable { /** * Check if the given target has a transaction open * - * @param target + * @param target the target * @return true if busy */ boolean isBusy(Target target); + + /** + * Forcibly commit all transactions this connection has on the given trace + * + *

+ * This may cause undefined behavior in the back-end, especially if it still needs the + * transaction. + * + * @param target the the target + */ + void forciblyCloseTransactions(Target target); } diff --git a/Ghidra/Debug/Debugger-rmi-trace/src/main/help/help/topics/TraceRmiConnectionManagerPlugin/TraceRmiConnectionManagerPlugin.html b/Ghidra/Debug/Debugger-rmi-trace/src/main/help/help/topics/TraceRmiConnectionManagerPlugin/TraceRmiConnectionManagerPlugin.html index 9547a6a869..1462f39b71 100644 --- a/Ghidra/Debug/Debugger-rmi-trace/src/main/help/help/topics/TraceRmiConnectionManagerPlugin/TraceRmiConnectionManagerPlugin.html +++ b/Ghidra/Debug/Debugger-rmi-trace/src/main/help/help/topics/TraceRmiConnectionManagerPlugin/TraceRmiConnectionManagerPlugin.html @@ -105,5 +105,15 @@

Stop the server. This closes the persistent server. This does not affect pending acceptors or established connections.

+ +

Forcibly Close Transactions

+ +

Forcibly close all the back-end's transactions on the target trace. This is generally not a + recommended course of action, except that sometimes the back-end crashes and fails to close a + transaction. Un-closed transactions from the back-end can leave most, if not all, of the UI in + a stale state, since event processing on the trace is disabled. If there is good reason to + believe the back-end has forgotten to close a transaction, this action will forcibly close all + of them and re-enable event processing. If, however, the back-end was in fact still doing work + with that transaction, it may crash and/or corrupt the connection.

diff --git a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/connection/TraceRmiConnectionManagerProvider.java b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/connection/TraceRmiConnectionManagerProvider.java index f821e59bf8..515a7306f6 100644 --- a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/connection/TraceRmiConnectionManagerProvider.java +++ b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/connection/TraceRmiConnectionManagerProvider.java @@ -161,6 +161,24 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter } } + interface ForceCloseTransactionsActions { + String NAME = "Forcibly Close Transactions"; + String DESCRIPTION = "Forcibly commit all remote transactions on the trace"; + String GROUP = GROUP_MAINTENANCE; + String HELP_ANCHOR = "forcibly_close_txes"; + + static ActionBuilder builder(Plugin owner) { + String ownerName = owner.getName(); + return new ActionBuilder(NAME, ownerName) + .description(DESCRIPTION) + .menuPath(NAME) + .popupMenuPath(NAME) + .menuGroup(GROUP) + .popupMenuGroup(GROUP) + .helpLocation(new HelpLocation(ownerName, HELP_ANCHOR)); + } + } + class InjectableGTree extends GTree { public InjectableGTree(GTreeNode root) { super(root); @@ -200,6 +218,7 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter DockingAction actionConnectOutbound; DockingAction actionCloseConnection; DockingAction actionCloseAll; + DockingAction actionForceCloseTransactions; TraceRmiManagerActionContext myActionContext; @@ -309,6 +328,12 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter .enabledWhen(this::isActionCloseAllEnabled) .onAction(this::doActionCloseAllActivated) .buildAndInstallLocal(this); + + actionForceCloseTransactions = ForceCloseTransactionsActions.builder(plugin) + .withContext(TraceRmiManagerActionContext.class) + .enabledWhen(this::isActionForceCloseTransactionsEnabled) + .onAction(this::doActionCloseTransactionsActivated) + .buildAndInstallLocal(this); } @Override @@ -482,6 +507,21 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter } } + private boolean isActionForceCloseTransactionsEnabled(TraceRmiManagerActionContext context) { + TraceRmiManagerNode node = context.getSelectedNode(); + if (node instanceof TraceRmiTargetNode) { + return true; + } + return false; + } + + private void doActionCloseTransactionsActivated(TraceRmiManagerActionContext context) { + TraceRmiManagerNode node = context.getSelectedNode(); + if (node instanceof TraceRmiTargetNode tNode) { + tNode.getTarget().forciblyCloseTransactions(); + } + } + @AutoServiceConsumed private void setTraceRmiService(TraceRmiService traceRmiService) { if (this.traceRmiService != null) { diff --git a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiHandler.java b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiHandler.java index 784d329075..1a7986fd9f 100644 --- a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiHandler.java +++ b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiHandler.java @@ -946,6 +946,18 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection { .build(); } + protected void checkRestoreEvents(OpenTrace open) { + final boolean restoreEvents; + synchronized (openTxes) { + restoreEvents = openTxes.keySet() + .stream() + .noneMatch(id -> id.doId.equals(open.doId)); + } + if (restoreEvents) { + open.trace.setEventsEnabled(true); + } + } + protected ReplyEndTx handleEndTx(RequestEndTx req) { OpenTx tx; synchronized (openTxes) { @@ -969,16 +981,8 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection { } tx.tx.close(); + checkRestoreEvents(open); - final boolean restoreEvents; - synchronized (openTxes) { - restoreEvents = openTxes.keySet() - .stream() - .noneMatch(id -> id.doId.domObjId == req.getOid().getId()); - } - if (restoreEvents) { - open.trace.setEventsEnabled(true); - } Swing.runLater( () -> plugin.listeners.invoke().transactionClosed(this, open.target, req.getAbort())); return ReplyEndTx.getDefaultInstance(); @@ -1325,7 +1329,9 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection { @Override public boolean isBusy() { - return !openTxes.isEmpty(); + synchronized (openTxes) { + return !openTxes.isEmpty(); + } } @Override @@ -1335,11 +1341,33 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection { return false; } - for (Tid tid : openTxes.keySet()) { - if (Objects.equals(openTrace.doId, tid.doId)) { - return true; + synchronized (openTxes) { + for (Tid tid : openTxes.keySet()) { + if (Objects.equals(openTrace.doId, tid.doId)) { + return true; + } } } return false; } + + @Override + public void forciblyCloseTransactions(Target target) { + OpenTrace open = openTraces.getByTrace(target.getTrace()); + if (open == null || open.target != target) { + return; + } + synchronized (openTxes) { + for (OpenTx tx : List.copyOf(openTxes.values())) { + if (Objects.equals(open.doId, tx.txId.doId)) { + openTxes.remove(tx.txId); + tx.tx.commit(); + tx.tx.close(); + Swing.runLater( + () -> plugin.listeners.invoke().transactionClosed(this, target, false)); + } + } + } + open.trace.setEventsEnabled(true); + } } diff --git a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiTarget.java b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiTarget.java index 779643b1a5..b12a86067c 100644 --- a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiTarget.java +++ b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiTarget.java @@ -1736,4 +1736,9 @@ public class TraceRmiTarget extends AbstractTarget { public boolean isBusy() { return connection.isBusy(this); } + + @Override + public void forciblyCloseTransactions() { + connection.forciblyCloseTransactions(this); + } } diff --git a/Ghidra/Debug/Debugger-rmi-trace/src/test/java/ghidra/app/plugin/core/debug/service/tracermi/TestTraceRmiConnection.java b/Ghidra/Debug/Debugger-rmi-trace/src/test/java/ghidra/app/plugin/core/debug/service/tracermi/TestTraceRmiConnection.java index 4a35aaae56..a67663468d 100644 --- a/Ghidra/Debug/Debugger-rmi-trace/src/test/java/ghidra/app/plugin/core/debug/service/tracermi/TestTraceRmiConnection.java +++ b/Ghidra/Debug/Debugger-rmi-trace/src/test/java/ghidra/app/plugin/core/debug/service/tracermi/TestTraceRmiConnection.java @@ -277,4 +277,8 @@ public abstract class TestTraceRmiConnection extends AbstractTraceRmiConnection public boolean isBusy(Target target) { return false; } + + @Override + public void forciblyCloseTransactions(Target target) { + } } diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/MockTarget.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/MockTarget.java index 4f0ebe5bd8..fbc4fc7b0a 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/MockTarget.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/MockTarget.java @@ -278,4 +278,8 @@ public class MockTarget implements Target { public boolean isBusy() { return false; } + + @Override + public void forciblyCloseTransactions() { + } }