Bug/12187 v5 reliably detect Suricata startup

[KEIAHNY]

PR Type

Bug fix, Enhancement

---

Description

• Replace process-name scan with pidfile-based PID detection

• Wait for Suricata readiness via suricatasc socket polling

• Add socket config option and -s CLI flag

• Delete stale pidfile before launching Suricata

---

Diagram Walkthrough

flowchart LR
  A["Delete stale pidfile"] -- "launch with --pidfile" --> B["Start Suricata"]
  B -- "read pidfile + check /proc" --> C["Get Suricata PID"]
  C -- "poll suricatasc uptime" --> D["Suricata ready"]
  C -- "timeout or error" --> E["Kill Suricata and panic"]

Loading

File Walkthrough

	Relevant files
Bug fix	suricata.rs Detect Suricata startup via pidfile and socket                      src/suricata.rs Add PIDFILE constant and pass --pidfile to Suricata command Add delete_pid_file() to remove stale pidfile before startup Replace check_process_name_for_suricata_main() with get_suricata_pid() reading the pidfile and verifying /proc/ Add wait_on_suricata_start() polling suricatasc -c uptime on the socket; kill Suricata and panic on failure Change child stdout from piped to null +78/-18
Enhancement	argument.rs Add CLI option for Suricata socket path                                    src/argument.rs Add -s/--path_to_socket CLI option to the Suricata command for overriding the Unix socket path +4/-0      yaml.rs Add socket path to Suriconf configuration                                src/yaml.rs Add socket: PathBuf field to Suriconf struct Resolve socket path from CLI argument or config file Add find_socket() parser for the socket YAML key +12/-0
Documentation	README.md Document socket configuration option                                          README.md Document the new socket configuration option in the config table +1/-0
Configuration changes	suriconf.yaml Add default socket path to config file                                      src/suriconf.yaml Add default socket: /var/run/suricata/suricata-command.socket entry +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 Backward Incompatibility The new socket field is required with no fallback default. Existing suriconf.yaml files that don't contain a socket key will make find_socket return None, and the .expect("Unable to parse path to unix socket.") call will panic on startup. Consider defaulting to the common /var/run/suricata/suricata-command.socket (or deriving it) when the key is absent, or documenting the migration. 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.") }; Possible Issue delete_pid_file unconditionally removes /var/run/suricata.pid via sudo rm -f. If a Suricata instance not managed by this tool is already running (e.g., on a different interface), its pidfile is silently deleted and a second Suricata is started, which then overwrites the pidfile. This can leave the original instance untrackable by PID and result in two Suricata processes running concurrently. pub fn delete_pid_file() -> std::io::Result<()> { let status = Command::new("sudo") .arg("-n") .arg("rm") .arg("-f") .arg(PIDFILE) .status()?; if status.success() { Ok(()) } else { Err(std::io::Error::other("Failed to delete Suricata PID file.")) } } Log Noise In wait_on_suricata_start, each failed suricatasc poll uses Command::status(), which inherits the process's stdout/stderr. While Suricata is starting, suricatasc connection errors will be printed to the console once per second for potentially up to 120 iterations, flooding output. Redirect the command's output (e.g., Stdio::null()) unless the errors are intended to be visible. let output = Command::new("sudo") .arg("-n") .arg("suricatasc") .arg("-c") .arg("uptime") .arg(socket) .status() .map_err(|e| format!("Unable to execute Suricata socket control tool: {e}"))?; if output.success() { return Ok(pid) } thread::sleep(Duration::from_millis(1000));

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Verify PID actually belongs to Suricata Checking only that /proc/{pid} exists can match an unrelated process if a stale PID file was not removed or a PID was reused, causing monitoring of the wrong process (as the old comm == "Suricata-Main" check guarded against). Verify the process name via /proc/{pid}/comm before returning the PID. src/suricata.rs [316-319] if let Ok(pid) = content.trim().parse::<i32>() { - if Path::new(&format!("/proc/{}", pid)).exists() { + let comm = fs::read_to_string(format!("/proc/{}/comm", pid)).unwrap_or_default(); + if comm.trim() == "Suricata-Main" { return Ok(pid); } Suggestion importance[1-10]: 6 __ Why: The PR removed the old comm == "Suricata-Main" verification, and the new PID-file approach could theoretically match an unrelated process via PID reuse or a stale file, so restoring the /proc/{pid}/comm check is an accurate and meaningful robustness improvement. Impact is moderate since delete_pid_file() and the short retry window make mismatches unlikely in practice.	Low
	Gate Unix-specific startup commands for Windows These Unix-specific sudo rm and --pidfile operations run before the cfg!(target_os = "windows") check, so on Windows the code would panic with a confusing sudo failure instead of hitting the intended todo!(). Guard these calls so they only execute on non-Windows targets. src/suricata.rs [40-48] let mut vec_of_sur_cmd: Vec<String> = vec![]; - match delete_pid_file() { - Err(e) => { - panic!("{e}"); - } - Ok(()) => {} - }; + if !cfg!(target_os = "windows") { + match delete_pid_file() { + Err(e) => { + panic!("{e}"); + } + Ok(()) => {} + }; - set_pid_file(&mut vec_of_sur_cmd); + set_pid_file(&mut vec_of_sur_cmd); + } Suggestion importance[1-10]: 6 __ Why: Correct observation: delete_pid_file() (which invokes sudo rm) runs before the cfg!(target_os = "windows") check, so Windows would panic with a confusing sudo error instead of reaching the intended todo!(). The fix is accurate, though impact is limited since Windows support is unimplemented anyway.	Low
	Fail fast if Suricata exits during startup wait If Suricata crashes during the 120-second startup wait, this loop keeps invoking suricatasc until the full timeout elapses, delaying the failure. Pass the spawned child into wait_on_suricata_start and use try_wait() to detect early exit and fail fast with the actual exit status. src/suricata.rs [340-345] if output.success() { return Ok(pid) + } + if let Some(status) = child.try_wait().expect("Unable to get Suricata status.") { + return Err(format!("Suricata exited during startup with status: {status}.")); } thread::sleep(Duration::from_millis(1000)); } Err("Suricata could not start.".into()) Suggestion importance[1-10]: 5 __ Why: The concern is valid: if Suricata fails immediately, the loop still polls suricatasc for the full 120 seconds before reporting failure, and checking the spawned child via try_wait() would enable early failure detection. However, the improved_code references child without showing the required signature change to wait_on_suricata_start, and child is the sudo wrapper rather than Suricata itself, slightly weakening the guarantee.	Low
	Make PID file wait timeout configurable The hardcoded 20-second window for Suricata to write its PID file may be too short on systems with large rule sets or heavy startup load, causing a spurious "Suricata process not found" failure. Make this timeout configurable (e.g., derived from preconf_time or a constant with a larger value) instead of a magic number. src/suricata.rs [314-315] - for _ in 0..20 { + let deadline = std::time::Instant::now() + Duration::from_secs(SURICATA_PIDFILE_TIMEOUT_SECS); + while std::time::Instant::now() < deadline { if let Ok(content) = fs::read_to_string(PIDFILE) { Suggestion importance[1-10]: 3 __ Why: The suggestion is technically valid but speculative: 20 seconds for Suricata to write its PID file is usually sufficient, and the subsequent wait_on_suricata_start already provides a separate 120-second window for full startup. This is a minor maintainability/style improvement with marginal impact.	Low

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