From 3d20a3104f5d7890a8481d625955605283b0294e Mon Sep 17 00:00:00 2001 From: Alexander Kiselev Date: Wed, 4 Feb 2026 14:26:33 -0800 Subject: [PATCH] fixes --- AGENTS.md | 12 +-- CHANGELOG.md | 22 ++++++ CLAUDE.md | 16 ++-- Cargo.lock | 60 ++++++++++++++- Cargo.toml | 1 + README.md | 57 +++++---------- src/cli.rs | 8 +- src/daemon/README.md | 162 +++++++++++++++++------------------------ src/daemon/mod.rs | 6 ++ src/ghidra/bridge.rs | 14 ++-- src/ipc/client.rs | 3 + src/ipc/transport.rs | 1 + src/main.rs | 41 +++++++++-- tests/README.md | 92 +++++++++++++---------- tests/common/README.md | 121 ++++++++++-------------------- 15 files changed, 333 insertions(+), 283 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4d62348..0165002 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,8 +7,10 @@ ## Architecture -ghidra-cli uses a **daemon-only architecture**: -- All commands route through a daemon process -- Daemon manages a persistent Ghidra bridge connection -- Import/Analyze/Quick commands auto-start the daemon -- One daemon per project +ghidra-cli uses a **direct bridge architecture**: +- CLI connects directly to a Java bridge running inside Ghidra's JVM via TCP +- The bridge is a GhidraScript (`GhidraCliBridge.java`) started via `analyzeHeadless -postScript` +- Bridge binds `ServerSocket(0)` on localhost, writes port/PID files for discovery +- One bridge per project, identified by `~/.local/share/ghidra-cli/bridge-{md5}.port` +- Import/Analyze/Quick commands auto-start the bridge if not running +- No separate Rust daemon process — the Java bridge IS the persistent server diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fdc9a9..0062ff1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING**: Replaced Python bridge (`bridge.py`) with Java bridge (`GhidraCliBridge.java`) + - Architecture simplified from 3 layers (CLI → Rust daemon → Python bridge) to 2 layers (CLI → Java bridge) + - No separate Rust daemon process — CLI connects directly to Java bridge via TCP + - Bridge runs as a GhidraScript inside `analyzeHeadless` JVM + - Dynamic port binding with port/PID file discovery (`~/.local/share/ghidra-cli/bridge-{hash}.port`) +- **BREAKING**: Removed Python/PyGhidra dependency — only Java 17+ and Ghidra are required +- `ghidra setup` no longer installs PyGhidra +- `ghidra doctor` no longer checks for Python/PyGhidra + +### Removed + +- All 13 Python scripts (`bridge.py`, `find.py`, `symbols.py`, `types.py`, `comments.py`, `graph.py`, `diff.py`, `patch.py`, `disasm.py`, `stats.py`, `program.py`, `script_runner.py`, `batch.py`) +- Rust daemon process and associated modules (`handler.rs`, `handlers/`, `ipc_server.rs`, `process.rs`, `queue.rs`, `state.rs`, `cache.rs`) +- Dependencies: `remoc`, `interprocess`, `fslock` +- Unix domain socket IPC — replaced with direct TCP to Java bridge + +### Security + +- Local TCP communication only (localhost binding, no external access) + ## [0.1.0] - 2025-01-26 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index e45ea9a..fde2d34 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,18 +6,23 @@ See @AGENTS.md for agent-specific instructions. | What | When | |------|------| -| `src/main.rs` | Modifying CLI entry point, daemon lifecycle, or output format detection | +| `src/main.rs` | Modifying CLI entry point, bridge lifecycle, or output format detection | | `src/cli.rs` | Adding/modifying CLI arguments and subcommands | | `src/format/mod.rs` | Implementing new output formats or changing format detection logic | -| `src/daemon/handlers/*.rs` | Implementing daemon command handlers | -| `PLAN.md` | Understanding current implementation plan or reviewing decision rationale | +| `src/ghidra/bridge.rs` | Bridge process management (start/stop/status/connect via TCP) | +| `src/ghidra/scripts/GhidraCliBridge.java` | Java bridge server (TCP, command handlers, Ghidra API) | +| `src/ipc/client.rs` | BridgeClient (TCP connection, command methods) | +| `src/ipc/protocol.rs` | BridgeRequest/BridgeResponse wire format | +| `PLAN-java-plugin.md` | Architecture decisions and migration rationale | | `README.md` | Understanding project architecture or user-facing command documentation | ## Modules | What | When | |------|------| -| `src/daemon/` | Working with persistent Ghidra daemon or IPC communication | +| `src/ghidra/` | Bridge management, Ghidra setup/installation, Java bridge script | +| `src/ipc/` | TCP client, protocol definitions, transport helpers | +| `src/daemon/` | Thin wrapper over bridge.rs (kept for API compatibility) | | `src/format/` | Handling output format conversion (Table, Compact, JSON, CSV, etc.) | | `tests/` | Writing integration or unit tests | @@ -25,7 +30,6 @@ See @AGENTS.md for agent-specific instructions. | What | When | |------|------| -| `CONTRIBUTING.md` | Setting up development environment, understanding PR process, test requirements | | `CHANGELOG.md` | Reviewing version history and release notes | -| `src/daemon/README.md` | Understanding daemon architecture and IPC protocol | +| `src/daemon/README.md` | Understanding bridge architecture and command protocol | | `tests/README.md` | Understanding test structure and conventions | diff --git a/Cargo.lock b/Cargo.lock index 47b466d..be359f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -380,6 +380,15 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "crossbeam-channel" +version = "0.5.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "82b8f8f868b36967f9606790d1903570de9ceaf870a7bf9fbbd3016d636a2cb2" +dependencies = [ + "crossbeam-utils", +] + [[package]] name = "crossbeam-utils" version = "0.8.21" @@ -774,9 +783,10 @@ dependencies = [ "strsim", "tabled", "tempfile", - "thiserror", + "thiserror 1.0.69", "tokio", "tracing", + "tracing-appender", "tracing-subscriber", "uuid", "walkdir", @@ -1719,7 +1729,7 @@ checksum = "ba009ff324d1fc1b900bd1fdb31564febe58a8ccc8a6fdbb93b543d33b13ca43" dependencies = [ "getrandom 0.2.17", "libredox", - "thiserror", + "thiserror 1.0.69", ] [[package]] @@ -2266,7 +2276,16 @@ version = "1.0.69" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6aaf5339b578ea85b50e080feb250a3e8ae8cfcdff9a461c9ec2904bc923f52" dependencies = [ - "thiserror-impl", + "thiserror-impl 1.0.69", +] + +[[package]] +name = "thiserror" +version = "2.0.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4288b5bcbc7920c07a1149a35cf9590a2aa808e0bc1eafaade0b80947865fbc4" +dependencies = [ + "thiserror-impl 2.0.18", ] [[package]] @@ -2280,6 +2299,17 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "thiserror-impl" +version = "2.0.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ebc4ee7f67670e9b64d05fa4253e753e016c6c95ff35b89b7941d6b856dec1d5" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.114", +] + [[package]] name = "thread_local" version = "1.1.9" @@ -2296,10 +2326,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f9e442fc33d7fdb45aa9bfeb312c095964abdf596f7567261062b2a7107aaabd" dependencies = [ "deranged", + "itoa", "num-conv", "powerfmt", "serde_core", "time-core", + "time-macros", ] [[package]] @@ -2308,6 +2340,16 @@ version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b36ee98fd31ec7426d599183e8fe26932a8dc1fb76ddb6214d05493377d34ca" +[[package]] +name = "time-macros" +version = "0.2.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71e552d1249bf61ac2a52db88179fd0673def1e1ad8243a00d9ec9ed71fee3dd" +dependencies = [ + "num-conv", + "time-core", +] + [[package]] name = "tinystr" version = "0.8.2" @@ -2382,6 +2424,18 @@ dependencies = [ "tracing-core", ] +[[package]] +name = "tracing-appender" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "786d480bce6247ab75f005b14ae1624ad978d3029d9113f0a22fa1ac773faeaf" +dependencies = [ + "crossbeam-channel", + "thiserror 2.0.18", + "time", + "tracing-subscriber", +] + [[package]] name = "tracing-attributes" version = "0.1.31" diff --git a/Cargo.toml b/Cargo.toml index b11dc29..d9b33da 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ env_logger = "0.11" log = "0.4" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } +tracing-appender = "0.2" # Async runtime (only needed for setup command's HTTP downloads) tokio = { version = "1.35", features = ["rt", "io-util", "time", "net"] } diff --git a/README.md b/README.md index 390ca57..1de045a 100644 --- a/README.md +++ b/README.md @@ -4,14 +4,14 @@ A high-performance Rust CLI for automating Ghidra reverse engineering tasks, des ## Features -- **Daemon-only architecture** - All operations route through a persistent daemon for consistency -- **Auto-start daemon** - Import/analyze commands automatically start the daemon +- **Direct bridge architecture** - CLI connects directly to a Java bridge running inside Ghidra's JVM +- **Auto-start bridge** - Import/analyze commands automatically start the bridge - **Fast queries** - Sub-second response times with Ghidra kept in memory - **Comprehensive analysis** - Functions, symbols, types, strings, cross-references - **Binary patching** - Modify bytes, NOP instructions, export patches - **Call graphs** - Generate caller/callee graphs, export to DOT format - **Search capabilities** - Find strings, bytes, functions, crypto patterns -- **Script execution** - Run Python/Java scripts, inline or from files +- **Script execution** - Run Java/Python Ghidra scripts, inline or from files - **Batch operations** - Execute multiple commands from a file - **Flexible output** - Human-readable, JSON, or pretty JSON formats - **Filtering** - Powerful expression-based filtering (e.g., `size > 100`) @@ -19,24 +19,19 @@ A high-performance Rust CLI for automating Ghidra reverse engineering tasks, des ## Architecture ``` -┌─────────────────┐ ┌──────────────────┐ ┌─────────────────┐ -│ CLI Command │────▶│ Daemon (IPC) │────▶│ GhidraBridge │ -│ ghidra ... │ │ Per-project │ │ TCP to Ghidra │ -│ --project X │ │ Unix socket │ │ │ -└─────────────────┘ └──────────────────┘ └─────────────────┘ - │ - ▼ - ┌─────────────────┐ - │ bridge.py │ - │ (Ghidra Script)│ - └─────────────────┘ +┌─────────────────┐ ┌──────────────────────────────────────┐ +│ CLI Command │──TCP──▶ │ GhidraCliBridge.java │ +│ ghidra ... │ │ (GhidraScript in analyzeHeadless) │ +│ --project X │ │ ServerSocket on localhost:dynamic │ +└─────────────────┘ └──────────────────────────────────────┘ ``` -All commands go through the daemon, which maintains a persistent connection to Ghidra via the bridge script. This provides: +The CLI connects directly to a Java bridge running inside Ghidra's JVM. This provides: - **Consistent state** - Single Ghidra process for all operations - **Fast queries** - No JVM startup overhead per command -- **Auto-start** - Daemon starts automatically when needed -- **Per-project isolation** - Each project gets its own daemon and socket, enabling concurrent analysis of multiple binaries +- **Auto-start** - Bridge starts automatically when needed +- **Per-project isolation** - Each project gets its own bridge process and port file, enabling concurrent analysis of multiple binaries +- **Minimal dependencies** - Only Ghidra + Java required (no Python/PyGhidra) ## Installation @@ -174,22 +169,22 @@ ghidra stats # Program statistics ghidra summary # Program summary ``` -## Daemon Management +## Bridge Management -The daemon keeps Ghidra loaded in memory. It starts automatically when needed, but you can also control it manually: +The bridge keeps Ghidra loaded in memory. It starts automatically when needed, but you can also control it manually: ```bash -# Start daemon with a program loaded +# Start bridge with a program loaded ghidra daemon start --project myproject --program mybinary -# Check daemon status +# Check bridge status ghidra daemon status --project myproject -# All commands use the daemon automatically +# All commands use the bridge automatically ghidra function list --project myproject # Fast! ghidra decompile main --project myproject # Fast! -# Stop daemon +# Stop bridge ghidra daemon stop --project myproject # Restart with different program @@ -198,7 +193,7 @@ ghidra daemon restart --project myproject --program otherbinary ### Multi-Project Support -Each project gets its own daemon process and socket, allowing concurrent analysis: +Each project gets its own bridge process and port file, allowing concurrent analysis: ```bash # Work on multiple projects simultaneously @@ -296,19 +291,7 @@ WSL requires X11 libraries even for headless operation because Java AWT is loade 1. Install X11 libraries (see above) 2. If using WSL1, consider upgrading to WSL2 for better compatibility -3. Logs are written to `~/.local/share/ghidra-cli/daemon.log` - -#### Viewing Detailed Logs - -For debugging issues, check the daemon log: - -```bash -# View recent log entries -tail -100 ~/.local/share/ghidra-cli/daemon.log - -# Follow log in real-time -tail -f ~/.local/share/ghidra-cli/daemon.log -``` +3. Bridge port/PID files are stored in `~/.local/share/ghidra-cli/` #### Running Doctor diff --git a/src/cli.rs b/src/cli.rs index 025f875..773d63b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,4 +1,4 @@ -use clap::{Args, Parser, Subcommand}; +use clap::{ArgAction, Args, Parser, Subcommand}; use serde::{Deserialize, Serialize}; #[derive(Parser)] @@ -8,9 +8,9 @@ pub struct Cli { #[command(subcommand)] pub command: Commands, - /// Enable verbose output - #[arg(short, long, global = true)] - pub verbose: bool, + /// Increase log verbosity printed to stdout (-v=warn, -vv=info, -vvv=debug) + #[arg(short, long, action = ArgAction::Count, global = true)] + pub verbose: u8, /// Suppress non-essential output #[arg(short, long, global = true)] diff --git a/src/daemon/README.md b/src/daemon/README.md index 6cf6453..4d20cc3 100644 --- a/src/daemon/README.md +++ b/src/daemon/README.md @@ -1,30 +1,24 @@ -# Daemon Module +# Bridge Architecture -The daemon is the central execution authority for ghidra-cli. All commands route through the daemon, which maintains a persistent connection to Ghidra via the bridge. +The bridge is the central execution layer for ghidra-cli. All commands route through a Java bridge running inside Ghidra's JVM, which maintains persistent access to the loaded program. ## Architecture ``` -┌─────────────┐ ┌─────────────┐ ┌─────────────┐ ┌─────────────┐ -│ CLI Client │────▶│ IPC Server │────▶│ Handler │────▶│ GhidraBridge│ -│ (DaemonCli) │ │ Per-project │ │ (Routing) │ │ (TCP→Ghidra)│ -│ │ │ Unix socket │ │ │ │ │ -└─────────────┘ └─────────────┘ └─────────────┘ └─────────────┘ - │ - ▼ - ┌─────────────┐ - │ bridge.py │ - │ (In Ghidra) │ - └─────────────┘ +┌─────────────┐ ┌──────────────────────────────────────┐ +│ CLI Client │──TCP──▶ │ GhidraCliBridge.java │ +│ (ghidra) │ │ (GhidraScript in analyzeHeadless JVM)│ +│ --project X │ │ ServerSocket on localhost:dynamic │ +└─────────────┘ └──────────────────────────────────────┘ ``` -### Per-Project Socket Isolation +### Per-Project Bridge Isolation -Each project gets its own Unix socket to enable concurrent daemon operation: +Each project gets its own bridge process with unique port/PID files: -- **Socket naming**: `ghidra-cli-{hash}.sock` where `{hash}` is MD5 of project path -- **Socket location**: `$XDG_RUNTIME_DIR/ghidra-cli/` or `/tmp/ghidra-cli/` -- **Lock file naming**: `daemon-{hash}.lock` (same hash for consistency) +- **Port file**: `~/.local/share/ghidra-cli/bridge-{hash}.port` — contains the TCP port number +- **PID file**: `~/.local/share/ghidra-cli/bridge-{hash}.pid` — contains the JVM process ID +- **Hash**: MD5 of the canonical project path This allows multiple agents or terminals to work on different projects without conflicts. @@ -32,109 +26,89 @@ This allows multiple agents or terminals to work on different projects without c | File | Purpose | |------|---------| -| `mod.rs` | Daemon main loop, startup, shutdown | -| `ipc_server.rs` | Unix socket server, accepts client connections | -| `handler.rs` | Routes IPC commands to bridge or specialized handlers | -| `process.rs` | Daemon lifecycle, lock files, process management | -| `queue.rs` | Command queue execution | -| `cache.rs` | Result caching | -| `state.rs` | Daemon state management | -| `handlers/` | Specialized command handlers | +| `src/ghidra/bridge.rs` | Bridge process management (start, stop, status, connect) | +| `src/ghidra/scripts/GhidraCliBridge.java` | Java bridge server (TCP, 17+ command handlers) | +| `src/ipc/client.rs` | BridgeClient — TCP connection, all command methods | +| `src/ipc/protocol.rs` | BridgeRequest/BridgeResponse structs (JSON wire format) | +| `src/ipc/transport.rs` | TCP transport helpers (port reachability check) | +| `src/daemon/mod.rs` | Thin wrapper over bridge.rs (kept for API compatibility) | ## Command Flow -1. **CLI sends command** via IPC (Unix socket) -2. **IPC server** receives request, parses JSON -3. **Handler** routes to appropriate processor: - - Direct bridge commands (decompile, function list, etc.) - - Import/Analyze commands (via bridge.py handlers) - - ExecuteCli for generic CLI command forwarding -4. **Bridge** sends to Ghidra via TCP, receives response -5. **Response** flows back through IPC to CLI +1. **CLI parses command** and resolves project path +2. **Bridge discovery**: read port file, verify PID alive, verify TCP connect +3. **Auto-start**: if bridge not running, spawn `analyzeHeadless -postScript GhidraCliBridge.java` +4. **Send command**: TCP connect to localhost:port, send `{"command":"...","args":{...}}\n` +5. **Receive response**: read `{"status":"success|error","data":{...},"message":"..."}\n` +6. **Format output**: CLI applies format transformation (human-readable, JSON, pretty) ## Auto-Start Behavior -Import, Analyze, and Quick commands auto-start the daemon: +Import, Analyze, and Quick commands auto-start the bridge: -1. CLI checks if daemon is running for project -2. If not, starts daemon in background (`daemonize_unix` / `daemonize_windows`) -3. Waits briefly for daemon to initialize -4. Connects and sends command +1. CLI reads port file for the project +2. If missing or stale (dead PID, TCP connect fails): launch `analyzeHeadless` with Java bridge +3. Bridge binds `ServerSocket(0)`, writes port + PID files, prints ready signal +4. CLI reads port from file, connects, sends command ## Lifecycle -- **One daemon per project** - Lock file prevents duplicates -- **One program per daemon** - Daemon loads a single program -- **Graceful shutdown** - Handles SIGTERM, SIGINT, IPC shutdown command -- **Lock files** - Located at `~/.local/share/ghidra-cli/daemon-{hash}.lock` -- **Socket files** - Located at `$XDG_RUNTIME_DIR/ghidra-cli/ghidra-cli-{hash}.sock` -- **Logs** - Located at `~/.local/share/ghidra-cli/daemon.log` +- **One bridge per project** — port file path includes project hash +- **Sequential command processing** — single accept loop, one connection at a time (Ghidra Program objects are not thread-safe) +- **Graceful shutdown** — `shutdown` command breaks accept loop, deletes port/PID files, `run()` returns, `analyzeHeadless` exits +- **Forced shutdown** — read PID file, kill process +- **Stale file cleanup** — on startup, detect dead PID + unreachable port → clean up files and start fresh -The `{hash}` is computed as `MD5(project_path_string)` ensuring each project has unique socket and lock file names. - -## Handlers - -Specialized handlers in `handlers/` directory: - -| Handler | Commands | -|---------|----------| -| `program.rs` | Program info, memory, imports, exports | -| `symbols.rs` | Symbol operations | -| `types.rs` | Data type operations | -| `comments.rs` | Comment operations | -| `graph.rs` | Call graph operations | -| `find.rs` | Search operations | -| `diff.rs` | Program diff operations | -| `patch.rs` | Binary patching | -| `script.rs` | Script execution | -| `disasm.rs` | Disassembly | -| `stats.rs` | Statistics | -| `batch.rs` | Batch command execution | +The `{hash}` is computed as `MD5(project_path_string)` ensuring each project has unique port and PID file names. ## Bridge Commands -Commands sent to bridge.py in Ghidra: +Commands handled by GhidraCliBridge.java: -- `import` - Import binary using AutoImporter -- `analyze` - Trigger analysis using AutoAnalysisManager -- `list_functions`, `decompile`, `list_strings`, etc. - -See `src/ghidra/scripts/bridge.py` for the full command reference. +| Category | Commands | +|----------|----------| +| Core | `ping`, `shutdown`, `status` | +| Program | `program_info`, `list_programs`, `open_program`, `program_close`, `program_delete`, `program_export` | +| Import/Analysis | `import`, `analyze` | +| Functions | `list_functions`, `decompile` | +| Data | `list_strings`, `list_imports`, `list_exports`, `memory_map` | +| Xrefs | `xrefs_to`, `xrefs_from` | +| Symbols | `symbol_list`, `symbol_get`, `symbol_create`, `symbol_delete`, `symbol_rename` | +| Types | `type_list`, `type_get`, `type_create`, `type_apply` | +| Comments | `comment_list`, `comment_get`, `comment_set`, `comment_delete` | +| Search | `find_string`, `find_bytes`, `find_function`, `find_calls`, `find_crypto`, `find_interesting` | +| Graph | `graph_calls`, `graph_callers`, `graph_callees`, `graph_export` | +| Diff | `diff_programs`, `diff_functions` | +| Patch | `patch_bytes`, `patch_nop`, `patch_export` | +| Disasm | `disasm` | +| Stats | `stats` | +| Scripts | `script_run`, `script_python`, `script_java`, `script_list` | +| Batch | `batch` | ## Reliability -### Bridge Health Monitoring +### Bridge Liveness Detection -The bridge (`GhidraBridge`) tracks whether the Ghidra JVM process is alive: +Bridge liveness is checked via three steps: -- **`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 +1. **Port file exists** — `bridge-{hash}.port` present in data directory +2. **PID alive** — `kill(pid, 0)` succeeds (Unix) for the PID in `bridge-{hash}.pid` +3. **TCP reachable** — `TcpStream::connect(("127.0.0.1", port))` succeeds -### 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. +If any check fails, stale files are cleaned up and a fresh bridge is started. ### Stale File Cleanup -On daemon startup, `get_running_daemon_info()` detects and cleans stale files: +On bridge startup, stale files from previous crashes are detected and removed: -- **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 +- **Port file** — removed if PID is dead or TCP unreachable +- **PID file** — removed alongside stale port file -This handles crash scenarios where daemon died without proper cleanup. +This handles crash scenarios where the bridge died without proper cleanup. ### Startup Logging -During bridge startup, all Ghidra stdout and stderr is captured and logged at `info` level: +During bridge startup, all Ghidra stdout and stderr is captured and logged: ``` [Ghidra stdout] INFO ANALYZING all memory and code: ... @@ -144,6 +118,4 @@ During bridge startup, all Ghidra stdout and stderr is captured and logged at `i This aids in diagnosing issues like: - Missing system libraries (X11 libs on Linux/WSL) - Java version mismatches -- PyGhidra initialization failures - -Logs are written to: `~/.local/share/ghidra-cli/daemon.log` +- GhidraScript compilation failures diff --git a/src/daemon/mod.rs b/src/daemon/mod.rs index 935b1cd..b94c1ba 100644 --- a/src/daemon/mod.rs +++ b/src/daemon/mod.rs @@ -12,6 +12,7 @@ use anyhow::Result; use crate::ghidra::bridge::{self, BridgeStartMode, BridgeStatus}; /// Bridge configuration (replaces old DaemonConfig). +#[allow(dead_code)] pub struct BridgeConfig { /// Path to the Ghidra project directory pub project_path: PathBuf, @@ -22,27 +23,32 @@ pub struct BridgeConfig { /// Ensure a bridge is running for the given project. /// If import mode, starts with the binary. If process mode, opens existing program. /// Returns the port number for connecting. +#[allow(dead_code)] pub fn ensure_bridge(config: &BridgeConfig, mode: BridgeStartMode) -> Result { bridge::ensure_bridge_running(&config.project_path, &config.ghidra_install_dir, mode) } /// Start a new bridge for the given project. /// Returns the port number for connecting. +#[allow(dead_code)] pub fn start_bridge(config: &BridgeConfig, mode: BridgeStartMode) -> Result { bridge::start_bridge(&config.project_path, &config.ghidra_install_dir, mode) } /// Stop the bridge for a project. +#[allow(dead_code)] pub fn stop_bridge(project_path: &Path) -> Result<()> { bridge::stop_bridge(project_path) } /// Get bridge status for a project. +#[allow(dead_code)] pub fn get_bridge_status(project_path: &Path) -> Result { bridge::bridge_status(project_path) } /// Check if a bridge is running for a project. +#[allow(dead_code)] pub fn is_bridge_running(project_path: &Path) -> bool { bridge::is_bridge_running(project_path) } diff --git a/src/ghidra/bridge.rs b/src/ghidra/bridge.rs index d225d2e..a493b37 100644 --- a/src/ghidra/bridge.rs +++ b/src/ghidra/bridge.rs @@ -246,11 +246,9 @@ pub fn start_bridge( let stderr_handle = std::thread::spawn(move || { let reader = BufReader::new(stderr); let mut stderr_output = Vec::new(); - for line in reader.lines() { - if let Ok(line) = line { - info!("[Ghidra stderr] {}", line); - stderr_output.push(line); - } + for line in reader.lines().map_while(Result::ok) { + info!("[Ghidra stderr] {}", line); + stderr_output.push(line); } stderr_output }); @@ -430,10 +428,8 @@ pub fn bridge_status(project_path: &Path) -> Result { let pid = read_pid_file(project_path)?; if let (Some(port), Some(pid)) = (port, pid) { - if is_pid_alive(pid) { - if TcpStream::connect(format!("127.0.0.1:{}", port)).is_ok() { - return Ok(BridgeStatus::Running { port, pid }); - } + if is_pid_alive(pid) && TcpStream::connect(format!("127.0.0.1:{}", port)).is_ok() { + return Ok(BridgeStatus::Running { port, pid }); } // Stale files cleanup_stale_files(project_path).ok(); diff --git a/src/ipc/client.rs b/src/ipc/client.rs index 84333f3..e2fd9d6 100644 --- a/src/ipc/client.rs +++ b/src/ipc/client.rs @@ -25,6 +25,7 @@ impl BridgeClient { } /// Get the port this client connects to. + #[allow(dead_code)] pub fn port(&self) -> u16 { self.port } @@ -82,12 +83,14 @@ impl BridgeClient { } /// Shutdown the bridge. + #[allow(dead_code)] pub fn shutdown(&self) -> Result<()> { self.send_command("shutdown", None)?; Ok(()) } /// Get bridge status. + #[allow(dead_code)] pub fn status(&self) -> Result { self.send_command("status", None) } diff --git a/src/ipc/transport.rs b/src/ipc/transport.rs index 36b140f..0d69121 100644 --- a/src/ipc/transport.rs +++ b/src/ipc/transport.rs @@ -6,6 +6,7 @@ use std::net::TcpStream; /// Check if a TCP port is reachable on localhost. +#[allow(dead_code)] pub fn port_reachable(port: u16) -> bool { TcpStream::connect(format!("127.0.0.1:{}", port)) .map(|_| true) diff --git a/src/main.rs b/src/main.rs index 9f38d79..9f92589 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,15 +17,46 @@ use ghidra::bridge::{self, BridgeStartMode, BridgeStatus}; use ghidra::GhidraClient; use ipc::client::BridgeClient; use std::path::{Path, PathBuf}; +use tracing_subscriber::layer::SubscriberExt; +use tracing_subscriber::util::SubscriberInitExt; +use tracing_subscriber::Layer; fn main() { - // Initialize logging with info level by default, can be overridden via RUST_LOG - let env_filter = tracing_subscriber::EnvFilter::try_from_default_env() - .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("info")); - tracing_subscriber::fmt().with_env_filter(env_filter).init(); - let cli = Cli::parse(); + // --- Logging setup --- + // File layer: always writes at debug level + let log_dir = dirs::data_local_dir() + .unwrap_or_else(|| PathBuf::from("/tmp")) + .join("ghidra-cli"); + let _ = std::fs::create_dir_all(&log_dir); + let file_appender = tracing_appender::rolling::daily(&log_dir, "ghidra-cli.log"); + let file_layer = tracing_subscriber::fmt::layer() + .with_writer(file_appender) + .with_ansi(false) + .with_filter(tracing_subscriber::EnvFilter::new("debug")); + + // Stdout layer: only if -v/-vv/-vvv is specified + let stdout_layer = match cli.verbose { + 1 => Some("warn"), + 2 => Some("info"), + 3.. => Some("debug"), + _ => None, + } + .map(|level| { + tracing_subscriber::fmt::layer() + .with_writer(std::io::stderr) + .with_filter( + tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new(level)), + ) + }); + + tracing_subscriber::registry() + .with(file_layer) + .with(stdout_layer) + .init(); + let result = match &cli.command { Commands::Setup(_) => { // Setup needs async for downloading diff --git a/tests/README.md b/tests/README.md index 0631cbb..ca6e4af 100644 --- a/tests/README.md +++ b/tests/README.md @@ -9,22 +9,35 @@ Tests are organized by functional area into separate files: ``` tests/ ├── common/ -│ └── mod.rs # DaemonTestHarness, fixtures, helpers -├── daemon_tests.rs # Daemon lifecycle: start/stop/restart/status/ping +│ ├── mod.rs # DaemonTestHarness, fixtures, helpers +│ ├── helpers.rs # GhidraCommand builder, GhidraResult assertions +│ └── schemas.rs # Response validation schemas +├── daemon_tests.rs # Bridge lifecycle: start/stop/restart/status/ping ├── project_tests.rs # Project: create/list/delete/info -├── query_tests.rs # Function/strings/memory/xref/dump queries +├── reliability_tests.rs # Bridge restart recovery, stale file cleanup ├── command_tests.rs # Basic commands: version/doctor/config/init +├── batch_tests.rs # Batch command execution +├── comment_tests.rs # Comment operations +├── diff_tests.rs # Program diff operations +├── find_tests.rs # Search operations +├── graph_tests.rs # Call graph operations +├── program_tests.rs # Program info/import/export +├── script_tests.rs # Script execution +├── stats_tests.rs # Statistics +├── symbol_tests.rs # Symbol operations +├── type_tests.rs # Type operations +├── output_format_integration.rs # Output format detection ├── unimplemented_tests.rs # Graceful error tests for stub commands └── e2e.rs # Lightweight smoke test ``` -## Per-Suite Daemon Lifecycle +## Per-Suite Bridge Lifecycle -Tests requiring daemon interaction use `DaemonTestHarness` from `common/mod.rs`. Each test suite starts its own daemon instance to amortize 5-30s startup overhead across all tests in that file. +Tests requiring bridge interaction use `DaemonTestHarness` from `common/mod.rs`. Each test suite starts its own bridge instance to amortize 5-30s startup overhead across all tests in that file. -**Why per-suite instead of per-test**: Starting a daemon for every test would add 5-30 minutes to CI time for 60+ tests. Per-suite daemons run tests serially within the suite but allow parallel execution across different test files. +**Why per-suite instead of per-test**: Starting a bridge for every test would add 5-30 minutes to CI time for 60+ tests. Per-suite bridges run tests serially within the suite but allow parallel execution across different test files. -**Why not shared global daemon**: State leakage between suites causes flaky tests and debugging nightmares. Each suite gets isolation. +**Why not shared global bridge**: State leakage between suites causes flaky tests and debugging nightmares. Each suite gets isolation. ## Data Flow @@ -34,8 +47,8 @@ Test Suite Start v DaemonTestHarness::new() | - +---> Start daemon process - +---> Wait for IPC socket + +---> Start bridge (analyzeHeadless + GhidraCliBridge.java) + +---> Wait for port file +---> Verify with ping | v @@ -44,9 +57,8 @@ Run tests (serial within suite) v DaemonTestHarness::drop() | - +---> Send shutdown command - +---> Wait for process exit - +---> Cleanup socket file + +---> Send shutdown command via TCP + +---> Cleanup port/PID files ``` ## Running Tests @@ -59,19 +71,17 @@ cargo test Run specific test suite: ```bash cargo test --test daemon_tests -cargo test --test query_tests cargo test --test command_tests ``` Run single test: ```bash -cargo test --test query_tests test_function_list +cargo test --test command_tests test_version ``` -Skip daemon tests (faster): +Run tests that don't need Ghidra: ```bash -cargo test --test command_tests -cargo test --test project_tests --lib +cargo test --test e2e --test command_tests --test output_format_integration ``` ## Test Requirements @@ -93,7 +103,7 @@ Fixture contains functions: add, multiply, factorial, fibonacci, process_string, ## Adding New Tests -### Non-Daemon Tests +### Non-Bridge Tests Add to appropriate file (`command_tests.rs`, `project_tests.rs`): @@ -110,9 +120,9 @@ fn test_my_command() { } ``` -### Daemon-Dependent Tests +### Bridge-Dependent Tests -Add to `daemon_tests.rs` or `query_tests.rs`: +Add to the appropriate test file (e.g., `symbol_tests.rs`, `find_tests.rs`): ```rust #[test] @@ -120,31 +130,37 @@ Add to `daemon_tests.rs` or `query_tests.rs`: fn test_my_query() { require_ghidra!(); - let harness = &*HARNESS; // Shared daemon instance + ensure_test_project(TEST_PROJECT, TEST_PROGRAM); + + let harness = + DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM).expect("Failed to start bridge"); Command::cargo_bin("ghidra") .unwrap() - .env("GHIDRA_CLI_SOCKET", harness.socket_path()) + .arg("--project") + .arg(TEST_PROJECT) .arg("my-query") - .arg("--project").arg(TEST_PROJECT) - .arg("--program").arg(TEST_PROGRAM) + .arg("--program") + .arg(TEST_PROGRAM) .assert() .success(); + + drop(harness); } ``` -Mark with `#[serial]` to prevent daemon state races within suite. +Mark with `#[serial]` to prevent bridge state races within suite. ## Invariants -- Daemon must be fully started before any query test runs -- Each test suite gets its own daemon instance (no sharing) -- Socket files must be cleaned up even on test failure (Drop impl) +- Bridge must be fully started before any query test runs +- Each test suite gets its own bridge instance (no sharing) +- Port/PID files must be cleaned up even on test failure (Drop impl) - Tests must not assume specific function addresses (use name-based lookups) ## Troubleshooting -### "Daemon failed to start within 120s timeout" +### "Bridge failed to start within timeout" Ghidra cold start can be slow. Ensure: - Ghidra installation is valid (`ghidra doctor`) @@ -158,11 +174,11 @@ Compile sample_binary: rustc --edition 2021 -o tests/fixtures/sample_binary tests/fixtures/sample_binary.rs ``` -### "Socket already in use" / "Address in use" +### "Port already in use" / stale port files -Each test suite generates UUID-based socket path to prevent collisions. This error suggests: -- Previous test run leaked daemon process (kill manually) -- Filesystem issue preventing socket cleanup +Each test suite uses project-name-based port file paths to prevent collisions. This error suggests: +- Previous test run leaked bridge process (kill manually) +- Stale port/PID files in `~/.local/share/ghidra-cli/` Find leaked processes: ```bash @@ -172,9 +188,9 @@ kill ### Tests hang or timeout -- Check if Ghidra daemon is stuck (check process list) -- Verify network/IPC permissions for Unix sockets -- Increase timeout in test (daemon startup can vary) +- Check if bridge is stuck (check process list) +- Verify TCP connectivity on localhost +- Increase timeout in test (bridge startup can vary) ### Import/analysis takes too long @@ -185,8 +201,8 @@ Tests use 300s timeout for import/analyze operations. On slow systems: ## Tradeoffs -**Per-suite vs per-test daemon**: Chose speed over maximum isolation. Tests within suite are serial, but suite-to-suite parallelism maintained. +**Per-suite vs per-test bridge**: Chose speed over maximum isolation. Tests within suite are serial, but suite-to-suite parallelism maintained. -**UUID socket paths vs fixed paths**: Chose reliability over simplicity. Guarantees uniqueness even with PID wrap on long-running CI. +**Project-name-based port files vs random ports**: Each test suite uses a unique project name which maps to a unique port file via MD5 hash, preventing collisions. **Testing unimplemented commands**: Adds maintenance burden but documents gaps and ensures graceful failures for stub commands. diff --git a/tests/common/README.md b/tests/common/README.md index 69f7dcc..c7178e2 100644 --- a/tests/common/README.md +++ b/tests/common/README.md @@ -4,7 +4,7 @@ Shared infrastructure for E2E tests. ## DaemonTestHarness -Manages daemon lifecycle for test suites requiring Ghidra daemon interaction. +Manages bridge lifecycle for test suites requiring Ghidra bridge interaction. ### Usage @@ -16,76 +16,41 @@ const TEST_PROGRAM: &str = "sample_binary"; #[test] #[serial] -fn test_with_daemon() { +fn test_with_bridge() { ensure_test_project(TEST_PROJECT, TEST_PROGRAM); let harness = DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM) - .expect("Failed to start daemon"); + .expect("Failed to start bridge"); - let mut client = harness.client().unwrap(); - // Use client for IPC calls + Command::cargo_bin("ghidra") + .unwrap() + .arg("--project") + .arg(TEST_PROJECT) + .arg("my-command") + .arg("--program") + .arg(TEST_PROGRAM) + .assert() + .success(); - // Daemon automatically shuts down when harness drops + // Bridge automatically shuts down when harness drops } ``` -### Shared Daemon Pattern +### Port File Discovery -For multiple tests in same suite: - -```rust -use once_cell::sync::Lazy; - -static HARNESS: Lazy = Lazy::new(|| { - ensure_test_project(TEST_PROJECT, TEST_PROGRAM); - DaemonTestHarness::new(TEST_PROJECT, TEST_PROGRAM) - .expect("Failed to start daemon") -}); - -#[test] -#[serial] -fn test_one() { - let harness = &*HARNESS; - // Use harness -} - -#[test] -#[serial] -fn test_two() { - let harness = &*HARNESS; - // Same daemon instance -} +Each harness discovers the bridge via port file: +``` +~/.local/share/ghidra-cli/bridge-{md5_hash}.port ``` -All tests using shared daemon must be marked `#[serial]` to prevent state races. - -### Why Runtime Field Exists - -`DaemonTestHarness` contains a `tokio::runtime::Runtime` field to: - -1. **Prevent panic-during-panic**: Creating Runtime during Drop panic unwinding causes abort. Pre-created runtime allows safe cleanup. -2. **Amortize overhead**: Runtime creation takes ~10ms. Reusing across all async operations saves time. - -### Socket Path Isolation - -Each harness instance generates UUID-based Unix socket path: -``` -/tmp/ghidra-test-.sock -``` - -UUID prevents collisions: -- Between parallel test suites -- Across test runs on long-running CI (PID can wrap) +Where `{md5_hash}` is derived from the canonical project path. The harness reads the port number from this file and connects via TCP. ### Cleanup Guarantees Drop implementation ensures best-effort cleanup: -1. Send shutdown via IPC (ignores errors) -2. Wait up to 5s for graceful exit -3. Kill process if still running -4. Remove socket file - -Accepts minor leak risk on panic-during-panic (rare edge case). +1. Send shutdown command via TCP (ignores errors) +2. Bridge deletes its own port/PID files on clean shutdown +3. Kill process via PID if still running ## Fixtures @@ -129,35 +94,29 @@ fn test_something() { Runs `ghidra doctor` and fails the test if Ghidra is unavailable, including doctor output. +## GhidraCommand Builder (helpers.rs) + +Fluent builder for constructing CLI commands in tests: + +```rust +use common::helpers::{ghidra, GhidraCommand}; + +let result = ghidra(&harness) + .arg("function") + .arg("list") + .run(); + +result.assert_success(); +``` + +The `ghidra(&harness)` helper pre-configures `--project` args from the harness. Additional helpers include `with_project()`, `json_format()`, and `timeout()`. + ## Design Decisions ### Exponential Backoff Parameters -`wait_for_ready()` uses: -- Initial delay: 100ms (responsive for fast starts) -- Multiplier: 2x -- Max attempts: 12 -- Total timeout: 120s - -Covers 100ms to ~200s range. Typical fast start exits in <5s. - -### ChildGuard Pattern - -`DaemonTestHarness::new()` uses ChildGuard to prevent daemon process leaks: - -```rust -struct ChildGuard(Option); -impl Drop for ChildGuard { - fn drop(&mut self) { - if let Some(mut child) = self.0.take() { - let _ = child.kill(); - } - } -} -``` - -If `wait_for_ready()` returns early due to error, ChildGuard ensures daemon process is killed. Without this, failed initialization leaks processes. +`wait_for_port()` uses backoff to wait for the bridge port file to appear after launching `analyzeHeadless`. Typical fast start exits in <5s. ### Why 5s Shutdown Timeout -Most daemons shut down in <1s. 5s allows graceful cleanup without blocking tests indefinitely. If daemon hangs, hard kill prevents test suite deadlock. +Most bridges shut down in <1s. 5s allows graceful cleanup without blocking tests indefinitely. If bridge hangs, hard kill via PID prevents test suite deadlock.