From 48754fb98dccff37b29f3b82a097a7e85f5c6bd1 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Sat, 6 Sep 2025 16:30:31 -0400 Subject: [PATCH 1/3] Test deadlock fix --- .../table/threaded/IncrementalLoadJob.java | 37 +++++++++++++++++-- .../table/threaded/ThreadedTableModel.java | 2 +- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java index 6113e9676b..fbd458cfe7 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java @@ -19,6 +19,7 @@ import static org.apache.commons.lang3.exception.ExceptionUtils.*; import java.util.Collection; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import ghidra.util.Msg; import ghidra.util.Swing; @@ -145,18 +146,48 @@ public class IncrementalLoadJob extends Job implements ThreadedTable Swing.runLater(() -> updateManager.addThreadedTableListener(IncrementalLoadJob.this)); } - waitForThreadedTableUpdateManagerToFinish(); + waitForThreadedTableUpdateManagerToFinish(monitor); } - private void waitForThreadedTableUpdateManagerToFinish() { + private void waitForThreadedTableUpdateManagerToFinish(TaskMonitor monitor) { + // + // This job can be stopped in a few ways: cancelled from the UI, interrupted from the + // update manager thread, or cancelled from an API call using the task monitor. There is a + // deadlock condition between the update manager and when this job waits on for the latch to + // be counted down. In this case, the update manager is attempting to start a new + // incremental load job while this job has not yet completed. If this job is waiting on + // the update manager callback while the update manager is waiting for this job to finish, + // then they both wait forever. It is not clear how this can happen, as this job should be + // interrupted before the update manager waits, which should trigger an interrupted + // exception when we wait on the countdown latch. There is a timing condition where this + // doesn't happen as expected. We suspect that when the interrupt does not happen, the job + // instead has been marked as cancelled. To catch this case, we need to periodically check + // the monitor. Thus, instead of waiting forever on the countdown latch, wait for a time + // period, checking the monitor for cancelled each time. + // + while (!hasBeenCancelled(monitor)) { + if (doWaitForThreadedTableUpdateManagerToFinish()) { + return; + } + } + } + + private boolean doWaitForThreadedTableUpdateManagerToFinish() { try { - completedCallbackLatch.await(); + // Don't wait forever. There is a deadlock condition where the latch is not being + // notified and this thread is not being interrupted. To deal with this case, + // periodically time out and let the caller of this method check for cancelled. If the + // latch is not counted down, then we still have not received a callback from the + // update manager. + completedCallbackLatch.await(5, TimeUnit.SECONDS); + return completedCallbackLatch.getCount() == 0; } catch (InterruptedException e) { // This implies the user has cancelled the job by starting a new one or that we have // been disposed. Whatever the cause, we want to let the control flow continue as // normal. Thread.currentThread().interrupt(); // preserve the interrupt status + return true; // interrupted means we have been cancelled } } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/ThreadedTableModel.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/ThreadedTableModel.java index 61756b8eff..6e2785b40c 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/ThreadedTableModel.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/ThreadedTableModel.java @@ -203,7 +203,7 @@ public abstract class ThreadedTableModel } private void cancelCurrentWorkerJob() { - if (worker != null && worker.isBusy()) { + if (worker != null) { worker.clearAllJobsWithInterrupt_IKnowTheRisks(); } } From deddc842055c304580c12b186080feb1887a4bc4 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Tue, 9 Sep 2025 11:18:24 -0400 Subject: [PATCH 2/3] Fix for previous commit; backport of master fix --- .../table/threaded/IncrementalLoadJob.java | 52 +++++-------------- 1 file changed, 13 insertions(+), 39 deletions(-) diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java index fbd458cfe7..b11de86a0c 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/IncrementalLoadJob.java @@ -19,7 +19,6 @@ import static org.apache.commons.lang3.exception.ExceptionUtils.*; import java.util.Collection; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; import ghidra.util.Msg; import ghidra.util.Swing; @@ -36,10 +35,9 @@ public class IncrementalLoadJob extends Job implements ThreadedTable /** * Used to signal that the updateManager has finished loading the final contents gathered - * by this job. By default, the value is 0, which means there is nothing to wait for. If we - * flush, this will be set to 1. + * by this job. This is also updated if this job is cancelled. */ - private volatile CountDownLatch completedCallbackLatch = new CountDownLatch(0); + private volatile CountDownLatch completedCallbackLatch = new CountDownLatch(1); private volatile boolean isCancelled = false; private volatile IncrementalUpdatingAccumulator incrementalAccumulator; @@ -141,53 +139,28 @@ public class IncrementalLoadJob extends Job implements ThreadedTable // -We release the lock // -A block on jobDone() can now complete as we release the lock // -jobDone() will notify listeners in an invokeLater(), which puts it behind ours - // - completedCallbackLatch = new CountDownLatch(1); + // Swing.runLater(() -> updateManager.addThreadedTableListener(IncrementalLoadJob.this)); } - waitForThreadedTableUpdateManagerToFinish(monitor); + waitForThreadedTableUpdateManagerToFinish(); } - private void waitForThreadedTableUpdateManagerToFinish(TaskMonitor monitor) { - // - // This job can be stopped in a few ways: cancelled from the UI, interrupted from the - // update manager thread, or cancelled from an API call using the task monitor. There is a - // deadlock condition between the update manager and when this job waits on for the latch to - // be counted down. In this case, the update manager is attempting to start a new - // incremental load job while this job has not yet completed. If this job is waiting on - // the update manager callback while the update manager is waiting for this job to finish, - // then they both wait forever. It is not clear how this can happen, as this job should be - // interrupted before the update manager waits, which should trigger an interrupted - // exception when we wait on the countdown latch. There is a timing condition where this - // doesn't happen as expected. We suspect that when the interrupt does not happen, the job - // instead has been marked as cancelled. To catch this case, we need to periodically check - // the monitor. Thus, instead of waiting forever on the countdown latch, wait for a time - // period, checking the monitor for cancelled each time. - // - while (!hasBeenCancelled(monitor)) { - if (doWaitForThreadedTableUpdateManagerToFinish()) { - return; - } - } - } - - private boolean doWaitForThreadedTableUpdateManagerToFinish() { + /** + * Waits for the final flushed data to be added to the table. We will get called when the data + * is finished loading or cancelled. The latch will also be released if the cancel method of + * this job is called. This can happen if the work queue is told to cancel all jobs, which can + * happen if a new reload job is requested. + */ + private void waitForThreadedTableUpdateManagerToFinish() { try { - // Don't wait forever. There is a deadlock condition where the latch is not being - // notified and this thread is not being interrupted. To deal with this case, - // periodically time out and let the caller of this method check for cancelled. If the - // latch is not counted down, then we still have not received a callback from the - // update manager. - completedCallbackLatch.await(5, TimeUnit.SECONDS); - return completedCallbackLatch.getCount() == 0; + completedCallbackLatch.await(); } catch (InterruptedException e) { // This implies the user has cancelled the job by starting a new one or that we have // been disposed. Whatever the cause, we want to let the control flow continue as // normal. Thread.currentThread().interrupt(); // preserve the interrupt status - return true; // interrupted means we have been cancelled } } @@ -210,6 +183,7 @@ public class IncrementalLoadJob extends Job implements ThreadedTable super.cancel(); isCancelled = true; incrementalAccumulator.cancel(); + completedCallbackLatch.countDown(); // Note: cannot do this here, since the cancel() call may happen asynchronously and after // a call to reload() on the table model. Assume that the model itself has already From bc6701a24dee2dcd8ff7217a51e83beac7070b86 Mon Sep 17 00:00:00 2001 From: LucaPalumbo Date: Thu, 14 Aug 2025 21:57:42 +0200 Subject: [PATCH 3/3] Fix off-by-one in ElfHeader.getSectionLoadHeaderContaining (Closes #8440) --- .../java/ghidra/app/util/bin/format/elf/ElfHeader.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/elf/ElfHeader.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/elf/ElfHeader.java index c0c6a13477..02ba8459c1 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/elf/ElfHeader.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/elf/ElfHeader.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. @@ -1606,14 +1606,13 @@ public class ElfHeader implements StructConverter { * @return the section header that contains the address */ public ElfSectionHeader getSectionLoadHeaderContaining(long address) { -// FIXME: verify for (ElfSectionHeader sectionHeader : sectionHeaders) { if (!sectionHeader.isAlloc()) { continue; } long start = sectionHeader.getAddress(); long end = start + sectionHeader.getSize(); - if (start <= address && address <= end) { + if (start <= address && address < end) { return sectionHeader; } }