From 9c189a32e9bddec4d157bf7a6c7f8d1f5f4f37ba Mon Sep 17 00:00:00 2001 From: Alexander Kiselev Date: Wed, 4 Feb 2026 02:24:40 -0800 Subject: [PATCH] daemon fixes and use cache dir instead of git --- src/config.rs | 8 +- src/daemon/README.md | 31 ++++++++ src/daemon/handler.rs | 41 ++++++---- src/daemon/mod.rs | 3 + src/daemon/process.rs | 2 + src/ghidra/bridge.rs | 57 ++++++++++++-- tests/reliability_tests.rs | 153 +++++++++++++++++++++++++++++++++++++ 7 files changed, 272 insertions(+), 23 deletions(-) create mode 100644 tests/reliability_tests.rs diff --git a/src/config.rs b/src/config.rs index 5d367b3..1b56c50 100644 --- a/src/config.rs +++ b/src/config.rs @@ -105,12 +105,12 @@ impl Config { return Ok(dir.clone()); } - // Default to ~/git - let home = dirs::home_dir().ok_or_else(|| { - GhidraError::ConfigError("Could not determine home directory".to_string()) + // Default to cache dir (e.g., ~/.cache/ghidra-cli/projects) + let cache_dir = dirs::cache_dir().ok_or_else(|| { + GhidraError::ConfigError("Could not determine cache directory".to_string()) })?; - Ok(home.join("git")) + Ok(cache_dir.join("ghidra-cli").join("projects")) } #[cfg(target_os = "windows")] diff --git a/src/daemon/README.md b/src/daemon/README.md index 040d855..db1c82c 100644 --- a/src/daemon/README.md +++ b/src/daemon/README.md @@ -100,3 +100,34 @@ Commands sent to bridge.py in Ghidra: - `list_functions`, `decompile`, `list_strings`, etc. See `src/ghidra/scripts/bridge.py` for the full command reference. + +## Reliability + +### Bridge Health Monitoring + +The bridge (`GhidraBridge`) tracks whether the Ghidra JVM process is alive: + +- **`check_health()`** - Uses `try_wait()` on the child process to detect if Ghidra has exited +- **`send_command()`** - On I/O errors, calls `check_health()` to distinguish process death from network timeouts +- **State update** - When process death is detected, `running` flag is set to false and daemon initiates shutdown + +### Daemon Termination on Bridge Death + +When the bridge process dies (Ghidra JVM crash, OOM, etc.): + +1. Handler detects "process died" error from bridge +2. Handler signals daemon shutdown via `shutdown_tx.send()` +3. Daemon performs graceful cleanup (socket, lock, info files) +4. Next CLI command auto-starts a fresh daemon + +This ensures clean state recovery without manual intervention. + +### Stale File Cleanup + +On daemon startup, `get_running_daemon_info()` detects and cleans stale files: + +- **Lock file** - If acquirable, previous daemon is dead; file removed +- **Info file** - Removed alongside stale lock file +- **Socket file** - Removed to prevent "address in use" errors + +This handles crash scenarios where daemon died without proper cleanup. diff --git a/src/daemon/handler.rs b/src/daemon/handler.rs index 496aef3..556922b 100644 --- a/src/daemon/handler.rs +++ b/src/daemon/handler.rs @@ -271,13 +271,13 @@ async fn handle_command_inner( // === Program management commands === Command::ListPrograms => { require_bridge(&state.bridge).await?; - execute_bridge_command(&state.bridge, "list_programs", None).await + execute_bridge_command(state, "list_programs", None).await } Command::OpenProgram { program } => { require_bridge(&state.bridge).await?; execute_bridge_command( - &state.bridge, + state, "open_program", Some(json!({"program": program})), ) @@ -288,7 +288,7 @@ async fn handle_command_inner( Command::ListFunctions { limit, filter } => { require_bridge(&state.bridge).await?; execute_bridge_command( - &state.bridge, + state, "list_functions", Some(json!({ "limit": limit, @@ -301,7 +301,7 @@ async fn handle_command_inner( Command::Decompile { address } => { require_bridge(&state.bridge).await?; execute_bridge_command( - &state.bridge, + state, "decompile", Some(json!({ "address": address, @@ -313,7 +313,7 @@ async fn handle_command_inner( Command::ListStrings { limit } => { require_bridge(&state.bridge).await?; execute_bridge_command( - &state.bridge, + state, "list_strings", Some(json!({ "limit": limit, @@ -324,28 +324,28 @@ async fn handle_command_inner( Command::ListImports => { require_bridge(&state.bridge).await?; - execute_bridge_command(&state.bridge, "list_imports", None).await + execute_bridge_command(state, "list_imports", None).await } Command::ListExports => { require_bridge(&state.bridge).await?; - execute_bridge_command(&state.bridge, "list_exports", None).await + execute_bridge_command(state, "list_exports", None).await } Command::MemoryMap => { require_bridge(&state.bridge).await?; - execute_bridge_command(&state.bridge, "memory_map", None).await + execute_bridge_command(state, "memory_map", None).await } Command::ProgramInfo => { require_bridge(&state.bridge).await?; - execute_bridge_command(&state.bridge, "program_info", None).await + execute_bridge_command(state, "program_info", None).await } Command::XRefsTo { address } => { require_bridge(&state.bridge).await?; execute_bridge_command( - &state.bridge, + state, "xrefs_to", Some(json!({ "address": address, @@ -357,7 +357,7 @@ async fn handle_command_inner( Command::XRefsFrom { address } => { require_bridge(&state.bridge).await?; execute_bridge_command( - &state.bridge, + state, "xrefs_from", Some(json!({ "address": address, @@ -384,12 +384,14 @@ async fn handle_command_inner( } /// Execute a command on the Ghidra bridge. +/// +/// If the bridge process dies during command execution, triggers daemon shutdown. async fn execute_bridge_command( - bridge: &Arc>>, + state: &Arc, command: &str, args: Option, ) -> anyhow::Result { - let mut bridge_guard = bridge.lock().await; + let mut bridge_guard = state.bridge.lock().await; let bridge = bridge_guard .as_mut() @@ -401,7 +403,18 @@ async fn execute_bridge_command( debug!("Executing bridge command: {}", command); - let response = bridge.send_command::(command, args)?; + let response = match bridge.send_command::(command, args) { + Ok(resp) => resp, + Err(e) => { + // Check if bridge process died - trigger daemon shutdown + let err_msg = e.to_string(); + if err_msg.contains("process died") || !bridge.is_running() { + info!("Bridge process died, triggering daemon shutdown"); + let _ = state.shutdown_tx.send(()); + } + return Err(e); + } + }; if response.status == "success" { Ok(response.data.unwrap_or(json!({}))) diff --git a/src/daemon/mod.rs b/src/daemon/mod.rs index 9762def..61041a7 100644 --- a/src/daemon/mod.rs +++ b/src/daemon/mod.rs @@ -41,6 +41,8 @@ pub struct DaemonState { pub ghidra_install_dir: Option, /// Project path on disk pub project_path: PathBuf, + /// Shutdown signal sender - handlers can trigger daemon shutdown on bridge death + pub shutdown_tx: broadcast::Sender<()>, } /// Run the daemon with the new bridge architecture. @@ -59,6 +61,7 @@ pub async fn run(config: DaemonConfig) -> Result<()> { bridge: Arc::new(Mutex::new(None)), ghidra_install_dir: config.ghidra_install_dir.clone(), project_path: config.project_path.clone(), + shutdown_tx: shutdown_tx.clone(), }); info!("Bridge will be started on first import/analyze command"); diff --git a/src/daemon/process.rs b/src/daemon/process.rs index e71f7f4..3563cf5 100644 --- a/src/daemon/process.rs +++ b/src/daemon/process.rs @@ -139,6 +139,8 @@ pub fn get_running_daemon_info(data_dir: &Path, project_path: &Path) -> Result Deserialize<'de>>( &mut self, command: &str, @@ -240,14 +243,29 @@ impl GhidraBridge { let request_json = serde_json::to_string(&request)?; debug!("Sending: {}", request_json); - // Send request - writeln!(stream, "{}", request_json)?; - stream.flush()?; + // Send request - check process health on I/O error + if let Err(e) = writeln!(stream, "{}", request_json) { + if !self.check_health() { + anyhow::bail!("Bridge process died unexpectedly"); + } + return Err(e.into()); + } + if let Err(e) = stream.flush() { + if !self.check_health() { + anyhow::bail!("Bridge process died unexpectedly"); + } + return Err(e.into()); + } - // Read response + // Read response - check process health on I/O error let mut reader = BufReader::new(stream.try_clone()?); let mut response_line = String::new(); - reader.read_line(&mut response_line)?; + if let Err(e) = reader.read_line(&mut response_line) { + if !self.check_health() { + anyhow::bail!("Bridge process died unexpectedly"); + } + return Err(e.into()); + } debug!("Received: {}", response_line.trim()); @@ -298,6 +316,35 @@ impl GhidraBridge { self.running.load(Ordering::SeqCst) } + /// Check if the bridge process is actually healthy (still running). + /// + /// Performs an OS-level check on the child process to detect if it + /// has exited unexpectedly. If the process has died, updates the running + /// flag and returns false. + pub fn check_health(&mut self) -> bool { + if let Some(ref mut child) = self.child { + match child.try_wait() { + Ok(None) => true, // Process still running + Ok(Some(status)) => { + // Process has exited + warn!("Bridge process exited with status: {}", status); + self.running.store(false, Ordering::SeqCst); + false + } + Err(e) => { + // Error checking process - assume dead + error!("Error checking bridge process health: {}", e); + self.running.store(false, Ordering::SeqCst); + false + } + } + } else { + // No child process + self.running.store(false, Ordering::SeqCst); + false + } + } + /// Get the embedded bridge script path, writing all scripts to disk. fn get_bridge_script_path(&self) -> Result { let scripts_dir = dirs::config_dir() diff --git a/tests/reliability_tests.rs b/tests/reliability_tests.rs new file mode 100644 index 0000000..31a8668 --- /dev/null +++ b/tests/reliability_tests.rs @@ -0,0 +1,153 @@ +//! Tests for daemon IPC reliability - bridge death detection and socket cleanup. + +use serial_test::serial; +use std::path::PathBuf; +use std::time::Duration; + +#[macro_use] +mod common; +use common::{ensure_test_project, DaemonTestHarness}; + +const TEST_PROJECT: &str = "reliability-test"; +const TEST_PROGRAM: &str = "sample_binary"; + +/// Test that stale socket files are cleaned up on daemon restart. +/// +/// Simulates a crash scenario where socket file remains but daemon is dead. +#[test] +#[serial] +fn test_stale_socket_cleaned_on_restart() { + require_ghidra!(); + + ensure_test_project(TEST_PROJECT, TEST_PROGRAM); + + // First daemon - start and stop cleanly + { + let harness = DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM) + .expect("Failed to start first daemon"); + + // Verify daemon is working + assert_cmd::Command::cargo_bin("ghidra") + .unwrap() + .env("GHIDRA_CLI_DATA_DIR", harness.data_dir()) + .env("GHIDRA_CLI_SOCKET", harness.socket_path()) + .arg("daemon") + .arg("ping") + .arg("--project") + .arg(TEST_PROJECT) + .timeout(Duration::from_secs(30)) + .assert() + .success(); + + // Drop will clean up + } + + // Brief pause to ensure cleanup completes + std::thread::sleep(Duration::from_millis(500)); + + // Second daemon - should start without issues (no stale socket conflict) + { + let harness = DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM) + .expect("Failed to start second daemon - stale socket may not have been cleaned"); + + // Verify daemon is working + assert_cmd::Command::cargo_bin("ghidra") + .unwrap() + .env("GHIDRA_CLI_DATA_DIR", harness.data_dir()) + .env("GHIDRA_CLI_SOCKET", harness.socket_path()) + .arg("daemon") + .arg("ping") + .arg("--project") + .arg(TEST_PROJECT) + .timeout(Duration::from_secs(30)) + .assert() + .success(); + } +} + +/// Test recovery after daemon crash (simulated via process kill). +/// +/// After killing daemon, a new daemon should be able to start successfully. +#[test] +#[serial] +fn test_recovery_after_crash() { + require_ghidra!(); + + ensure_test_project(TEST_PROJECT, TEST_PROGRAM); + + let socket_path: PathBuf; + let data_dir: PathBuf; + + // Start daemon and get its paths + { + let harness = DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM) + .expect("Failed to start daemon"); + + socket_path = harness.socket_path().to_path_buf(); + data_dir = harness.data_dir().to_path_buf(); + + // Verify it's working + assert_cmd::Command::cargo_bin("ghidra") + .unwrap() + .env("GHIDRA_CLI_DATA_DIR", &data_dir) + .env("GHIDRA_CLI_SOCKET", &socket_path) + .arg("daemon") + .arg("ping") + .arg("--project") + .arg(TEST_PROJECT) + .timeout(Duration::from_secs(30)) + .assert() + .success(); + + // Harness drop will kill daemon (simulating crash) + } + + // Brief pause + std::thread::sleep(Duration::from_millis(1000)); + + // New daemon should start successfully after crash cleanup + { + let harness = DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM) + .expect("Failed to start daemon after crash - cleanup may have failed"); + + // Verify new daemon is working + assert_cmd::Command::cargo_bin("ghidra") + .unwrap() + .env("GHIDRA_CLI_DATA_DIR", harness.data_dir()) + .env("GHIDRA_CLI_SOCKET", harness.socket_path()) + .arg("daemon") + .arg("ping") + .arg("--project") + .arg(TEST_PROJECT) + .timeout(Duration::from_secs(30)) + .assert() + .success(); + } +} + +/// Test that daemon commands return appropriate errors when bridge is not ready. +#[test] +#[serial] +fn test_bridge_not_ready_error() { + require_ghidra!(); + + ensure_test_project(TEST_PROJECT, TEST_PROGRAM); + + let harness = + DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM).expect("Failed to start daemon"); + + // Ping should work (doesn't require bridge) + assert_cmd::Command::cargo_bin("ghidra") + .unwrap() + .env("GHIDRA_CLI_DATA_DIR", harness.data_dir()) + .env("GHIDRA_CLI_SOCKET", harness.socket_path()) + .arg("daemon") + .arg("ping") + .arg("--project") + .arg(TEST_PROJECT) + .timeout(Duration::from_secs(30)) + .assert() + .success(); + + drop(harness); +}