Bug/12187 v3 reliably detect Suricata startup

[KEIAHNY]

PR Type

Bug fix, Enhancement

---

Description

• Add PID file handling for reliable Suricata detection

• Implement socket-based startup verification mechanism

• Add socket path configuration via CLI and YAML

• Replace process name scanning with PID file check

---

Diagram Walkthrough

flowchart TD
  A["Delete old PID file"] --> B["Set PID file argument"]
  B --> C["Start Suricata process"]
  C --> D["Wait for PID file"]
  D --> E["Verify socket connectivity"]
  E --> F["Confirm startup success"]

Loading

File Walkthrough

	Relevant files
Enhancement	argument.rs Add socket path command line argument                                        src/argument.rs Added -s/--path-to-socket CLI argument Enables custom Unix socket path configuration +4/-0      yaml.rs Add socket field to Suriconf configuration                              src/yaml.rs Added socket field to Suriconf struct Implemented find_socket() method for YAML parsing Updated configuration loading to handle socket path +12/-0
Bug fix	suricata.rs Implement reliable Suricata startup detection                        src/suricata.rs Added PID file constant and management functions Replaced process name scanning with PID file verification Implemented wait_on_suricata_start() with socket connectivity check Changed stdout from piped to null for cleaner output +70/-17
Configuration changes	suriconf.yaml Add socket path to default configuration                                  src/suriconf.yaml Added default socket path configuration Sets /var/run/suricata/suricata-command.socket as default +1/-0

---

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue delete_pid_file runs without sudo, but PIDFILE is /var/run/suricata.pid, which Suricata (running as root via sudo) creates. If the tool itself is executed by a non-root user and a stale pid file exists from a previous run, fs::remove_file fails with permission denied, and the code panics before Suricata is even started. Consider ignoring permission errors, or removing the file via a sudo call consistent with how Suricata is launched. pub fn delete_pid_file() -> std::io::Result<()> { match fs::remove_file(PIDFILE) { Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), result => result, } } Orphaned Process On startup-verification failure, child.kill() is called before panicking, but child is the sudo -n wrapper process, not Suricata itself. With default sudo behavior, killing sudo does not terminate the Suricata child, so Suricata keeps running (as root, capturing traffic) after this tool has panicked and exited. Killing the process group or the pid obtained from the pid file would be needed to actually stop Suricata. let suri_pid = match wait_on_suricata_start(&suriconf.socket) { Ok(pid) => pid, Err(e) => { let _ = child.kill(); panic!("{e}") } }; Config Mismatch Risk The socket path used for suricatasc is taken from this tool's own configuration (CLI -s or socket in suriconf.yaml), but the socket Suricata actually creates is defined in the Suricata configuration file referenced by suri_configuration. If the two differ (e.g., Suricata uses its default suricata-command.socket while the tool's yaml points elsewhere), suricatasc will never connect, the 10-second retry loop fails, and the tool panics with "Suricata could not start" even though Suricata started successfully. Consider documenting/validating that this must match Suricata's unix-command.filename. self.socket = if let Some(Commands::Suricata { path_to_socket: Some(p), .. }) = &args.cmd { p.clone() } else { self.find_socket(suriconf_string).expect("Unable to parse path to unix socket.") };

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Kill the real Suricata process on startup failure child is the PID of the sudo wrapper, not the Suricata process, so child.kill() will typically leave the root-owned Suricata process running after a failed startup. Since the pidfile was already read, kill the actual Suricata PID (e.g., via sudo -n kill ) in addition to the wrapper before panicking. src/suricata.rs [95-101] let suri_pid = match wait_on_suricata_start(&suriconf.socket) { Ok(pid) => pid, Err(e) => { let _ = child.kill(); + if let Ok(pid) = get_suricata_pid() { + let _ = Command::new("sudo").arg("-n").arg("kill") + .arg(pid.to_string()).status(); + } panic!("{e}") } }; Suggestion importance[1-10]: 7 __ Why: child is the sudo wrapper process, and since Child::kill() sends SIGKILL (which sudo cannot forward), the root-owned Suricata process would indeed keep running after a startup failure. The suggestion correctly addresses a real cleanup bug on the error path, though it only matters in the failure case.	Medium
	Validate pidfile PID belongs to Suricata The /proc/{} existence check can succeed for a PID reused by an unrelated process, and a partially written pidfile could parse a truncated PID. Prefer validating the process actually is Suricata (e.g., via Process::new(pid) and checking stat().comm == "Suricata-Main"), which also avoids the hardcoded /proc path dependency. src/suricata.rs [305-317] fn get_suricata_pid() -> Result<i32, String> { -for _ in 0..10 { - if let Ok(content) = fs::read_to_string(PIDFILE) { - if let Ok(pid) = content.trim().parse::<i32>() { - if Path::new(&format!("/proc/{}", pid)).exists() { - return Ok(pid); + for _ in 0..10 { + if let Ok(content) = fs::read_to_string(PIDFILE) { + if let Ok(pid) = content.trim().parse::<i32>() { + if let Ok(process) = Process::new(pid) { + if let Ok(stat) = process.stat() { + if stat.comm == "Suricata-Main" { + return Ok(pid); + } + } + } } } + thread::sleep(Duration::from_millis(1000)); } - thread::sleep(Duration::from_millis(1000)); -} -Err("Suricata process not found.".into()) + Err("Suricata process not found.".into()) } Suggestion importance[1-10]: 4 __ Why: PID reuse or a partially written pidfile are theoretically possible, but the PR already deletes a stale pidfile at startup (delete_pid_file) and the retry window is short, making this a low-probability edge case. The improved code is valid but partially reintroduces the process-name scanning the PR intentionally removed.	Low
General	Verify socket readiness more robustly suricatasc may exit with status 0 even when the command fails (e.g., socket connection errors are often reported on stdout rather than via the exit code). Consider capturing output and verifying the uptime value is parseable, or check that the socket file exists before invoking the tool, to avoid falsely detecting a successful startup. src/suricata.rs [323-336] + if !socket.as_ref().exists() { + thread::sleep(Duration::from_millis(1000)); + continue; + } let output = Command::new("sudo") .arg("-n") .arg("suricatasc") .arg("-c") .arg("uptime") .arg("--socket") .arg(socket) - .status() + .output() .map_err(|e| format!("Unable to execute Suricata socket control tool: {e}"))?; - if output.success() { + if output.status.success() && String::from_utf8_lossy(&output.stdout).contains("uptime") { return Ok(pid) } thread::sleep(Duration::from_millis(1000)); Suggestion importance[1-10]: 5 __ Why: The concern is valid: suricatasc can exit successfully even when the socket command fails, so checking the socket file's existence and output content is more robust. The improved code is reasonable but the stdout substring check (contains("uptime")) is a somewhat brittle heuristic, limiting its impact.	Low
	Use doc comment for CLI argument help Use a doc comment (///) instead of a regular comment (//) for this field, matching the adjacent arguments. With //, clap will not include this option's description in the generated --help output. src/argument.rs [71-73] - // Change path to Unix Socket + /// Change path to Unix Socket #[clap(short='s', long)] path_to_socket: Option<PathBuf>, Suggestion importance[1-10]: 3 __ Why: Correct and consistent with adjacent fields — // instead of /// means clap won't show this option's description in --help. However, it is only a minor documentation/help-text improvement with no functional impact.	Low

• Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.