Bug/12187 v7 reliably detect Suricata startup

[KEIAHNY]

PR Type

Bug fix, Enhancement

---

Description

• Reliably detect Suricata startup via PID file and socket

• Add path_to_socket CLI option and socket config field

• Replace process-name scan with PID file verification

• Kill Suricata and panic when startup fails

---

Diagram Walkthrough

flowchart LR
  start["Start Suricata"] -- "delete stale PID file" --> launch["Launch with --pidfile"]
  launch -- "read PID file + verify comm" --> pid["Get Suricata PID"]
  pid -- "poll suricatasc uptime on socket" --> ready["Suricata ready"]
  pid -- "startup failure" --> kill["Kill Suricata and panic"]

Loading

File Walkthrough

	Relevant files
Enhancement	argument.rs Add CLI option for Suricata socket path                                    src/argument.rs Add path_to_socket CLI option (-s) for Suricata command Allows overriding the path to the Suricata unix socket +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 option or config via new find_socket() +12/-0
Bug fix	suricata.rs Wait for Suricata startup via PID file and socket                src/suricata.rs Add PIDFILE constant and pass --pidfile to Suricata via set_pid_file() Delete stale PID file before launch with delete_pid_file() Replace check_process_name_for_suricata_main() with get_suricata_pid() reading the PID file and verifying /proc//comm 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 +89/-17
Documentation	README.md Document socket configuration option                                          README.md Document 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 path /var/run/suricata/suricata-command.socket +1/-0

---

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Compile Error The function get_suricata_pid is declared as fn get_suricata_pid() -> Result<i32, String> with no parameters, but its body references child (via child.try_wait()), which is undefined in this scope. This will not compile. The function needs a child: &mut Child parameter, and the call site in wait_on_suricata_start must pass child accordingly. fn get_suricata_pid() -> Result<i32, String> { for _ in 0..20 { if let Ok(content) = fs::read_to_string(PIDFILE) { if let Ok(pid) = content.trim().parse::<i32>() { if let Ok(comm) = fs::read_to_string(format!("/proc/{}/comm", pid)) { if comm.trim() == "Suricata-Main" { 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 process not found.".into()) } PID File Race In get_suricata_pid, the PID is read from the pidfile and verified against /proc/{pid}/comm of a process named Suricata-Main, but there is no verification that this PID belongs to the child process spawned by this tool. If another Suricata instance is running (or starts) on the same machine while the pidfile has not yet been written, this function can return the PID of an unrelated Suricata process. The subsequent monitoring and kill_suricata logic would then act on the wrong process. Comparing against the child's PID (after sudo exec, they should match) would make this robust. fn get_suricata_pid() -> Result<i32, String> { for _ in 0..20 { if let Ok(content) = fs::read_to_string(PIDFILE) { if let Ok(pid) = content.trim().parse::<i32>() { if let Ok(comm) = fs::read_to_string(format!("/proc/{}/comm", pid)) { if comm.trim() == "Suricata-Main" { 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 process not found.".into()) } Breaking Config Change The new socket field is now required: existing configuration files without a socket key (and no -s CLI flag) will hit the .expect("Unable to parse path to unix socket.") and panic. Since Suricata has a well-known default socket path (/var/run/suricata/suricata-command.socket), falling back to that default instead of panicking would preserve backward compatibility for existing users. 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.") }; Startup Error Handling In wait_on_suricata_start, if the suricatasc binary is missing from PATH, Command::output() returns an Err which is immediately propagated via ?, producing the error "Unable to execute Suricata socket control tool" and killing Suricata. This is distinct from Suricata not yet being ready. That is arguably correct behavior, but note that if suricatasc exists yet consistently fails (e.g., wrong socket path in config), the loop retries for the full 120 seconds before erroring; a shorter failure for a definitive command-level failure would speed up diagnostics. fn wait_on_suricata_start(socket: &PathBuf, child: &mut Child) -> Result<i32, String> { match get_suricata_pid() { Ok(pid) => { let socket = socket.to_str().ok_or("Socket path is not valid UTF-8.")?; for _ in 0..120 { let output = Command::new("sudo") .arg("-n") .arg("suricatasc") .arg("-c") .arg("uptime") .arg(socket) .output() .map_err(|e| format!("Unable to execute Suricata socket control tool: {e}"))?; if output.status.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()) } Err(e) => { Err(e) } }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Pass child process to startup detection function get_suricata_pid references child, but it is not a parameter of the function, so this will not compile. Pass the child process handle as a parameter (child: &mut Child) and forward it from wait_on_suricata_start. This also allows the early-exit detection to work correctly. src/suricata.rs [313-326] -fn get_suricata_pid() -> Result<i32, String> { +fn get_suricata_pid(child: &mut Child) -> Result<i32, String> { for _ in 0..20 { if let Ok(content) = fs::read_to_string(PIDFILE) { if let Ok(pid) = content.trim().parse::<i32>() { if let Ok(comm) = fs::read_to_string(format!("/proc/{}/comm", pid)) { if comm.trim() == "Suricata-Main" { - return Ok(pid); + 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}.")); - } + } + ... +} +fn wait_on_suricata_start(socket: &PathBuf, child: &mut Child) -> Result<i32, String> { + match get_suricata_pid(child) { + ... + } +} + Suggestion importance[1-10]: 9 __ Why: get_suricata_pid uses child.try_wait() without child being a parameter, which is a compile error in the new code. The suggestion correctly fixes this by passing the child handle through from wait_on_suricata_start, addressing a critical bug.	High
General	Validate socket tool response content suricatasc can exit with status 0 even when the socket command fails (e.g., returning an error message in its output). Verify the command's textual response in addition to the exit status, for example by checking that the output does not contain an error indicator, to avoid treating a failed startup as successful. src/suricata.rs [346-348] - if output.status.success() { - return Ok(pid) + let stdout = String::from_utf8_lossy(&output.stdout); + if output.status.success() && !stdout.contains("error") && !stdout.contains("failed") { + return Ok(pid); } Suggestion importance[1-10]: 5 __ Why: It is plausible that suricatasc returns exit status 0 even when the socket command fails, so checking the output content adds robustness to startup detection. However, the string-matching heuristic ("error"/"failed") is a somewhat crude and non-rigorous validation.	Low
	Propagate startup failure instead of panicking Panicking here unwinds the process while the spawned stderr-reader thread and the shared kill flag may not be finalized cleanly. Instead of panic!, prefer returning an error (or propagating None) so the caller can shut down threads and the child process in an orderly fashion. src/suricata.rs [95-101] let suri_pid = match wait_on_suricata_start(&suriconf.socket, &mut child) { Ok(pid) => pid, Err(e) => { kill_suricata(&mut child); - panic!("{e}") + return None; } }; Suggestion importance[1-10]: 5 __ Why: Returning None instead of panicking after kill_suricata leverages the existing Option return type and allows cleaner shutdown. It is a reasonable error-handling improvement, though the panic does kill the child first, so the impact is moderate.	Low
	Preserve Suricata stdout for diagnostics Discarding Suricata's stdout with Stdio::null() loses all diagnostic output that Suricata writes to stdout, making startup failures hard to diagnose. Consider redirecting stdout to a log file (e.g., under logs) or keeping it piped with a dedicated reader thread, mirroring the existing stderr handling. src/suricata.rs [77-83] + let stdout_log = std::fs::File::create(logs.log_dir.join("suricata_stdout.log")) + .expect("Unable to create Suricata stdout log file."); let mut child = Command::new("sudo") .arg("-n") .args(args) - .stdout(Stdio::null()) + .stdout(Stdio::from(stdout_log)) .stderr(Stdio::piped()) .spawn() .expect("Failed to execute process."); Suggestion importance[1-10]: 4 __ Why: The observation that Stdio::null() discards diagnostic output is valid, though the PR intentionally removed the piped stdout. The proposed fix references logs.log_dir which may not exist on CreatedLogs, making the improved code somewhat speculative, but the general direction improves debuggability.	Low

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