Nixyri/ghidra
1
0
Fork 0
mirror of https://github.com/NationalSecurityAgency/ghidra.git synced 2024-11-25 05:32:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

GP-4100: Fix issues between Debugger and NavHistory plugins.

Browse Source
This commit is contained in:
Dan 2024-01-03 12:29:38 -05:00
parent aec8641320
commit c3164a1902
11 changed files with 332 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/launcher/UnixShellScriptTraceRmiLaunchOffer.java
View File

@ -37,10 +37,13 @@ public class UnixShellScriptTraceRmiLaunchOffer extends AbstractScriptTraceRmiLa
/**
* Create a launch offer from the given shell script.
*
* @param plugin the launcher service plugin
* @param program the current program, usually the target image. In general, this should be used
* for at least two purposes. 1) To populate the default command line. 2) To ensure
* the target image is mapped in the resulting target trace.
* @throws FileNotFoundException
* @param script the script file that implements this offer
* @return the offer
* @throws FileNotFoundException if the script file does not exist
*/
public static UnixShellScriptTraceRmiLaunchOffer create(TraceRmiLauncherServicePlugin plugin,
Program program, File script) throws FileNotFoundException {

1
Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiHandler.java
View File

@ -225,6 +225,7 @@ public class TraceRmiHandler implements TraceRmiConnection {
* <p>
* Note it is common for this to be constructed by a TCP <em>client</em>.
*
* @param plugin the Trace RMI plugin
* @param socket the socket to the back-end debugger
* @throws IOException if there is an issue with the I/O streams
*/

2
Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointMarkerPlugin.java
View File

@ -973,9 +973,11 @@ public class DebuggerBreakpointMarkerPlugin extends Plugin
protected void removeMarkers(Program program) {
synchronized (markersByProgram) {
BreakpointMarkerSets oldSets = markersByProgram.remove(program);
if (oldSets != null) {
oldSets.dispose();
}
}
}
protected void doMarks(BreakpointMarkerSets marks,
Map<Address, Set<LogicalBreakpoint>> byAddress,

10
Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingProvider.java
View File

@ -434,8 +434,8 @@ public class DebuggerListingProvider extends CodeViewerProvider {
* is synchronized exactly with the other providers. "Main" on the other hand, does not
* necessarily have that property, but it is still <em>not</em> a snapshot. It is the main
* listing presented by this plugin, and so it has certain unique features. Calling
* {@link DebuggerListingPlugin#getConnectedProvider()} will return the main dynamic listing,
* despite it not really being "connected."
* {@link DebuggerListingPlugin#getProvider()} will return the main dynamic listing, despite it
* not really being "connected."
*
* @return true if this is the main listing for the plugin.
*/
@ -649,8 +649,12 @@ public class DebuggerListingProvider extends CodeViewerProvider {
@Override
protected void doSetProgram(Program newProgram) {
// E.g., The "Navigate Previous" could cause a change in trace
if (newProgram != null && current.getView() != null && newProgram != current.getView()) {
throw new AssertionError();
if (!(newProgram instanceof TraceProgramView view)) {
throw new IllegalArgumentException("Dynamic Listings require trace views");
}
traceManager.activateTrace(view.getTrace());
}
if (getProgram() == newProgram) {
return;

3
Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/memory/DebuggerMemoryBytesProvider.java
View File

@ -426,6 +426,9 @@ public class DebuggerMemoryBytesProvider extends ProgramByteViewerComponentProvi
current = coordinates;
addNewListeners();
doSetProgram(current.getView());
// NB. Also avoid a stale location being reported to the history service.
setLocation(null);
setSelection(null, false);
goToTrait.goToCoordinates(coordinates);
trackingTrait.goToCoordinates(coordinates);
readsMemTrait.goToCoordinates(coordinates);

8
Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java
View File

@ -75,7 +75,10 @@ import ghidra.util.task.*;
packageName = DebuggerPluginPackage.NAME,
status = PluginStatus.RELEASED,
eventsProduced = {
TraceOpenedPluginEvent.class,
TraceActivatedPluginEvent.class,
TraceInactiveCoordinatesPluginEvent.class,
TraceClosedPluginEvent.class,
},
eventsConsumed = {
TraceActivatedPluginEvent.class,
@ -284,6 +287,8 @@ public class DebuggerTraceManagerServicePlugin extends Plugin
private DebuggerPlatformService platformService;
// @AutoServiceConsumed via method
private DebuggerControlService controlService;
@AutoServiceConsumed
private NavigationHistoryService navigationHistoryService;
@SuppressWarnings("unused")
private final AutoService.Wiring autoServiceWiring;
@ -959,6 +964,9 @@ public class DebuggerTraceManagerServicePlugin extends Plugin
}
protected void doTraceClosed(Trace trace) {
if (navigationHistoryService != null) {
navigationHistoryService.clear(trace.getProgramView());
}
synchronized (listenersByTrace) {
trace.release(this);
lastCoordsByTrace.remove(trace);

17
Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/NavigationHistoryPlugin.java
View File

@ -348,7 +348,8 @@ public class NavigationHistoryPlugin extends Plugin
notifyHistoryChange();
}
private void clear(Program program) {
@Override
public void clear(Program program) {
for (HistoryList historyList : historyListMap.values()) {
clear(historyList, program);
}
@ -657,7 +658,7 @@ public class NavigationHistoryPlugin extends Plugin
ProgramLocation location =
validateProgramLocation(activeNavigatable, memento.getProgramLocation());
if (location != null) {
// prefer the active natigatable's location
// prefer the active navigatable's location
return location;
}
}
@ -683,11 +684,13 @@ public class NavigationHistoryPlugin extends Plugin
Program program = location.getProgram();
if (program.isClosed()) {
Msg.showError(this, null, "Closed Program",
"The Navigation History Plugin is using a closed program.\n" + "Program: " +
program.getName() + "Navigatable: " +
navigatable.getClass().getSimpleName() + "\n" + "Location: " +
location.getClass().getSimpleName() + " @ " + location.getAddress());
Msg.showError(this, null, "Closed Program", """
The Navigation History Plugin is using a closed program.
Program: %s
Navigatable: %s
Location: %s @ %s"""
.formatted(program.getName(), navigatable.getClass().getSimpleName(),
location.getClass().getSimpleName(), location.getAddress()));
return true;
}

59
Ghidra/Features/Base/src/main/java/ghidra/app/services/NavigationHistoryService.java
View File

@ -21,25 +21,29 @@ import ghidra.app.nav.LocationMemento;
import ghidra.app.nav.Navigatable;
import ghidra.app.plugin.core.navigation.NavigationHistoryPlugin;
import ghidra.framework.plugintool.ServiceInfo;
import ghidra.program.model.listing.Program;
/**
* The ToolStateHistoryService maintains a stack of locations that the user
* has visited via a navigation plugin.
* It provides methods querying and manipulating this list.
* The NavigationHistoryService maintains a stack of locations that the user has visited via a
* navigation plugin. It provides methods querying and manipulating this list.
*/
@ServiceInfo(defaultProvider = NavigationHistoryPlugin.class, description = "Maintains a history of tool states")
@ServiceInfo(
defaultProvider = NavigationHistoryPlugin.class,
description = "Maintains a history of tool states")
public interface NavigationHistoryService {
/**
* Positions the current location to the next location in the history list.
* If there is no "next" location, the history list remains unchanged.
* Positions the current location to the next location in the history list. If there is no
* "next" location, the history list remains unchanged.
*
* @param navigatable the navigatable to be navigated
*/
public void next(Navigatable navigatable);
/**
* Positions the "current" location to the previous location in the history list.
* If there is no "previous" location, the history list remains unchanged.
* Positions the "current" location to the previous location in the history list. If there is no
* "previous" location, the history list remains unchanged.
*
* @param navigatable the navigatable to be navigated
*/
public void previous(Navigatable navigatable);
@ -54,8 +58,8 @@ public interface NavigationHistoryService {
public void next(Navigatable navigatable, LocationMemento location);
/**
* Navigates to the given location in the "previous" list. If the location is not in
* the list, then nothing will happen
* Navigates to the given location in the "previous" list. If the location is not in the list,
* then nothing will happen
*
* @param navigatable the navigatable to be navigated
* @param location The location within the "previous" list to which to go.
@ -63,23 +67,25 @@ public interface NavigationHistoryService {
public void previous(Navigatable navigatable, LocationMemento location);
/**
* Positions the "current" location to the next location which is in a different function
* from current one or previous non-code location.
* If we are not inside any function, performs like "next".
* Positions the "current" location to the next location which is in a different function from
* current one or previous non-code location. If we are not inside any function, performs like
* "next".
*
* @param navigatable the navigatable to be navigated
*/
public void nextFunction(Navigatable navigatable);
/**
* Positions the "previous" location to the next location which is in a different function
* from current one or previous non-code location.
* If we are not inside any function, performs like "next".
* Positions the "previous" location to the next location which is in a different function from
* current one or previous non-code location. If we are not inside any function, performs like
* "next".
*
* @param navigatable the navigatable to be navigated
*/
public void previousFunction(Navigatable navigatable);
/**
* Returns the LocationMemento objects in the "previous" list
* Returns the {@link LocationMemento} objects in the "previous" list
*
* @param navigatable the navigatable to be navigated
* @return the LocationMemento objects in the "previous" list
@ -87,7 +93,7 @@ public interface NavigationHistoryService {
public List<LocationMemento> getPreviousLocations(Navigatable navigatable);
/**
* Returns the LocationMemento objects in the "next" list
* Returns the {@link LocationMemento} objects in the "next" list
*
* @param navigatable the navigatable to be navigated
* @return the LocationMemento objects in the "next" list
@ -112,6 +118,7 @@ public interface NavigationHistoryService {
/**
* Returns true if there is a valid "next" function location in the history list
*
* @param navigatable Navigatable object we are looking at
* @return true if there is a valid "next" function location
*/
@ -119,23 +126,31 @@ public interface NavigationHistoryService {
/**
* Returns true if there is a valid "previous" function location in the history list
*
* @param navigatable Navigatable object we are looking at
* @return true if there is a valid "previous" function location
*/
public boolean hasPreviousFunction(Navigatable navigatable);
/**
* Adds the given locationMomento to the list of previous locations. Clears the list
* of next locations.
* Adds the current location memento to the list of previous locations for the given
* navigatable. Clears the list of next locations.
*
* @param navigatable the navigatable to be navigated
*/
public void addNewLocation(Navigatable navigatable);
/**
* Removes all visited locations from the history list
* Removes all visited locations from the history list for the given navigatable
*
* @param navigatable the navigatable to be navigated
* @param navigatable the navigatable whose list to be cleared
*/
public void clear(Navigatable navigatable);
/**
* Removes all entries for the given program from all history lists
*
* @param program the program whose entries to be cleared
*/
public void clear(Program program);
}

2
Ghidra/Features/ByteViewer/src/main/java/ghidra/app/plugin/core/byteviewer/ProgramByteViewerComponentProvider.java
View File

@ -170,7 +170,7 @@ public class ProgramByteViewerComponentProvider extends ByteViewerComponentProvi
return getCurrentTextSelection();
}
private void setSelection(ProgramSelection selection, boolean notify) {
protected void setSelection(ProgramSelection selection, boolean notify) {
currentSelection = selection;
if (selection == null) {
return;

65
Ghidra/Framework/Docking/src/main/java/docking/WindowActionManager.java
View File

@ -16,11 +16,15 @@
package docking;
import java.util.*;
import java.util.Map.Entry;
import javax.swing.JMenuBar;
import docking.action.DockingActionIf;
import docking.menu.*;
import generic.concurrent.ReentryGuard;
import generic.concurrent.ReentryGuard.Guarded;
import ghidra.util.Msg;
public class WindowActionManager {
private Map<DockingActionIf, DockingActionProxy> actionToProxyMap;
@ -30,6 +34,24 @@ public class WindowActionManager {
private boolean disposed;
/**
* Some actions' {@link DockingActionIf#isEnabledForContext(ActionContext)} methods may
* inadvertently display an error dialog, allowing for the Swing thread to reenter and modify
* the {@link #actionToProxyMap}. We want to allow this, but detect it and bail early.
*/
private ReentryGuard<Throwable> reentryGuard = new ReentryGuard<>() {
@Override
protected Throwable violated(boolean nested, Throwable previous) {
if (previous != null) {
return previous;
}
Throwable t = new Throwable();
Msg.error(WindowActionManager.this,
"Modified action list during context change update", t);
return t;
}
};
WindowActionManager(WindowNode node, MenuHandler menuBarHandler,
DockingWindowManager winMgr, MenuGroupMap menuGroupMap) {
@ -40,6 +62,7 @@ public class WindowActionManager {
}
void setActions(List<DockingActionIf> actionList) {
reentryGuard.checkAccess();
menuBarMgr.clearActions();
toolBarMgr.clearActions();
actionToProxyMap.clear();
@ -49,6 +72,7 @@ public class WindowActionManager {
}
void addAction(DockingActionIf action) {
reentryGuard.checkAccess();
if (action.getMenuBarData() != null || action.getToolBarData() != null) {
DockingActionProxy proxyAction = new DockingActionProxy(action);
actionToProxyMap.put(action, proxyAction);
@ -58,6 +82,7 @@ public class WindowActionManager {
}
void removeAction(DockingActionIf action) {
reentryGuard.checkAccess();
DockingActionProxy proxyAction = actionToProxyMap.remove(action);
if (proxyAction != null) {
menuBarMgr.removeAction(proxyAction);
@ -80,6 +105,7 @@ public class WindowActionManager {
}
void dispose() {
reentryGuard.checkAccess();
disposed = true;
node.setToolBar(null);
node.setMenuBar(null);
@ -90,28 +116,42 @@ public class WindowActionManager {
void contextChanged(Map<Class<? extends ActionContext>, ActionContext> defaultContextMap,
ActionContext localContext, Set<DockingActionIf> excluded) {
/**
* We need the guard against reentrant changes to the actionToProxyMap, lest the iterator
* throw a ConcurrentModificationException. If the guard finds a violation, i.e., the map
* has changed, we just bail. Whatever changed the map should also trigger an update to
* context, and so we should have a fresh go here with the new map.
*
* There are three points where there could be reentrant modifications. Those are 1) when
* computing an action's context, 2) when checking if the action is enabled for that
* context, and 3) when setting the proxy's enabled state. There are minimal performance
* trade-offs to consider when deciding which points to check. Critically, we must check
* somewhere between the last point and the end of the loop, i.e., before stepping the
* iterator. We opt to check only that last point, since reentry is uncommon here.
*/
try (Guarded guarded = reentryGuard.enter()) {
if (!node.isVisible() || disposed) {
return;
}
// Update actions - make a copy so that we don't get concurrent modification
// exceptions during reentrant operations
List<DockingActionIf> list = new ArrayList<>(actionToProxyMap.keySet());
for (DockingActionIf action : list) {
for (Entry<DockingActionIf, DockingActionProxy> ent : actionToProxyMap.entrySet()) {
DockingActionIf action = ent.getKey();
DockingActionIf proxyAction = ent.getValue();
if (excluded.contains(action)) {
continue;
}
DockingActionIf proxyAction = actionToProxyMap.get(action);
// Reentry point 1
ActionContext context =
getContextForAction(action, localContext, defaultContextMap);
if (context != null) {
proxyAction.setEnabled(proxyAction.isEnabledForContext(context));
// Reentry point 2
boolean enabled =
context == null ? false : proxyAction.isEnabledForContext(context);
// Reentry point 3, which we check
proxyAction.setEnabled(enabled);
if (reentryGuard.isViolated()) {
break;
}
else {
proxyAction.setEnabled(false);
}
}
}
@ -134,7 +174,8 @@ public class WindowActionManager {
/**
* Returns the set of actions for this window.
*
* <p>Note this returns the the original passed-in actions and not the proxy actions that the
* <p>
* Note this returns the the original passed-in actions and not the proxy actions that the
* window uses.
*
* @return the set of actions for this window

185
Ghidra/Framework/Generic/src/main/java/generic/concurrent/ReentryGuard.java Normal file
View File

@ -0,0 +1,185 @@
/* ###
* IP: GHIDRA
*
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package generic.concurrent;
/**
* A means of detecting and handling reentrant conditions.
*
* <p>
* One example where this has been applied deals with updating actions upon changes in context. If,
* in the course of determining which actions are enabled, one of the {@code isEnabled} methods
* displays an error dialog, the Swing thread reenters its main loop while that dialog is showing,
* but before {@code isEnabled} has returned. This can cause all sorts of unexpected behaviors.
* Namely, a timer could fire, context could change again, etc., and the list of actions being
* updated may also change. At worst, this could result in many exceptions being thrown, because a
* data structure has been modified concurrently. At best, if the loop is allowed to finish, there's
* a lot of wasted time updating actions that will never be displayed.
*
* <p>
* In that example, the loop that updates the actions would be the "guarded block." Any point at
* which the list of actions is modified might result in "reentrant access" and should be checked.
*
* <p>
* This class provides a primitive for instrumenting, detecting, and properly reacting to such
* conditions. For example, if the modification should not be allowed at all, the guard can throw an
* exception at the moment of reentrant access. Alternatively, if the modification should be
* allowed, the guard would simply set a flag, then the guarded block can check that flag and
* terminate early.
*
* <p>
* This implementation is <em>not</em> thread safe. It is designed to check for reentrant access,
* not concurrent access. The client must ensure that only one thread enters the guarded block or
* calls {@link #checkAccess()} at a time. Otherwise, the behavior is undefined.
*
* <pre>
* public class ActionManager {
* private final ReentryGuard&lt;Throwable&gt; reentryGuard = new ReentryGuard&lt;&gt;() {
* &#64;Override
* public Throwable violated(boolean nested, Throwable previous) {
* if (previous != null) {
* return previous;
* }
* return new Throwable(); // record the stack of the violation
* }
* };
* private final List&lt;Action&gt; actions;
*
* public void addAction(Action action) {
* // Notify the guard we've committed some reentrant behavior.
* // Would need to add this to anything that modifies the action list.
* reentryGuard.checkAccess();
* actions.add(action);
* }
*
* public void updateActions(Context ctx) {
* try (Guarded guarded = reentryGuard.enter()) {
* // There is no need to create a copy, since we'll bail before the next iteration
* for (Action action : actions) {
* boolean enabled = action.isEnabledForContext(ctx);
* if (reentryGuard.getViolation() != null) {
* break; // Actions has been modified. Bail.
* // NOTE: This leaves the update incomplete.
* // Something has to call updateActions again.
* }
* actions.setEnabled(enabled);
* }
* }
* }
* }
* </pre>
*
* @param <T> the type used to record information about a violation. It cannot be {@link Void}.
*/
public abstract class ReentryGuard<T> {
private final Guarded guarded = new Guarded(this);
private boolean inGuarded;
private T violation;
public static class Guarded implements AutoCloseable {
private ReentryGuard<?> guard;
public Guarded(ReentryGuard<?> guard) {
this.guard = guard;
}
@Override
public void close() {
guard.inGuarded = false;
}
}
/**
* Notify the guard of entry into the guarded block
*
* <p>
* This should always be used in a {@code try-with-resources} block. This will ensure that the
* guard is notified of exit from the guarded block, even in exceptional circumstances.
*
* <p>
* NOTE: Re-entering the guarded portion is itself a violation.
*
* @return a closeable for notifying the guard of exit from the guarded block, or null if
* reentering the guarded block
*/
public Guarded enter() {
if (inGuarded) {
violation = violated(true, violation);
return null;
}
inGuarded = true;
violation = null;
return guarded;
}
/**
* Notify the guard of access to some resource used by the guarded block
*
* <p>
* If the access turns out to be reentrant, i.e., the thread's current stack includes a frame in
* the guarded block, this will call {@link #violated(boolean, Object)} and record the result.
* It can be inspected later via {@link #getViolation()}.
*/
public void checkAccess() {
if (inGuarded) {
violation = violated(false, violation);
}
}
/**
* Record a violation
*
* <p>
* This method is called if {@link #checkAccess()} or {@link #enter()} is called while already
* inside a guarded block. Its return value is stored for later inspection by
* {@link #getViolation()}. It's possible multiple violations occur within one execution of the
* guarded block. The previous return value of this method is provided, if that is the case. To
* record only the first violation, this method should just {@code return previous} when it is
* non-null. To record only the last violation, this method should disregard {@code previous}.
* To record all violations, {@code T} will need to be a collection, and this method will need
* to create and/or append to the collection.
*
* @param nested true if the violation is a nested call to {@link #enter()}; false if the
* violation is a call to {@link #checkAccess()}
* @param previous the previous return value of this method, on the occasion of multiple
* violations
* @return the record of the violation
*/
protected abstract T violated(boolean nested, T previous);
/**
* Retrieve a violation, if applicable
*
* <p>
* Calling this method outside of a guarded block has undefined behavior.
*
* @return the violation; or null to indicate no violation
*/
public T getViolation() {
return violation;
}
/**
* Check if there is a violation
*
* <p>
* This is equivalent to checking if {@link #getViolation()} returns non-null.
*
* @return true if there is a violation.
*/
public boolean isViolated() {
return violation != null;
}
}