Bug/12187 v8 reliably detect Suricata startup

[KEIAHNY]

PR Type

Bug fix, Enhancement

---

Description

• Reliably detect Suricata startup via pidfile and socket

  • New wait_on_suricata_start validates pidfile and polls suricatasc
  • Detects early Suricata exit and aborts with error

• Add configurable socket path option

  • New -s/path_to_socket CLI flag and socket YAML key

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

• Document new socket option in README and example config

---

Diagram Walkthrough

flowchart LR
  A["Spawn Suricata with --pidfile"] -- "wait & validate" --> B["Read pidfile + check /proc comm"]
  B -- "poll suricatasc uptime" --> C["Unix socket ready"]
  B -- "child exited" --> D["Kill & panic with error"]
  C -- "return pid" --> E["Continue preconfiguration run"]

Loading

File Walkthrough

	Relevant files
Bug fix	suricata.rs Wait for 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 wait_on_suricata_start, validating pidfile, /proc//comm, and polling suricatasc -c uptime on the socket Detect early child exit during startup, kill Suricata and panic on failure; change stdout to Stdio::null() +96/-17
Enhancement	argument.rs Add CLI flag for Suricata socket path                                        src/argument.rs Add new -s/path_to_socket CLI option for the Suricata command to override the Unix socket path +4/-0      yaml.rs Add socket path to Suriconf configuration                                src/yaml.rs Add socket field to Suriconf struct Resolve socket path from CLI argument or config via new find_socket parser +12/-0
Documentation	README.md Document socket configuration option                                          README.md Document the new socket configuration option in the YAML settings table +1/-0
Configuration changes	suriconf.yaml Add socket path to example configuration                                  src/suriconf.yaml Add default socket: /var/run/suricata/suricata-command.socket entry to example configuration +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 Startup Timeout Too Short The pidfile wait loop in wait_on_suricata_start gives up after only 20 seconds (20 x 1s sleeps) and returns "Suricata process not found.", even though the child process is still alive and healthy. Suricata writes its pidfile after initialization, which can take well over 20 seconds on systems with large rule sets or slow storage. In that scenario the caller panics and kills a perfectly healthy Suricata startup, which defeats the goal of "reliably detect Suricata startup". Consider making the timeout proportional to preconf_time or otherwise configurable. 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" { suri_pid = Some(pid); } } } } if let Some(status) = child.try_wait().expect("Unable to get Suricata status.") { return Err(format!("Suricata exited during startup with status: {status}.")); } if suri_pid.is_some() { break; } thread::sleep(Duration::from_millis(1000)); } let pid = match suri_pid { Some(pid) => pid, None => return Err("Suricata process not found.".into()), Misleading Socket Poll Failure The socket-readiness loop treats any non-success exit of suricatasc the same as "socket not ready yet" and retries for 120 seconds. If suricatasc is not permitted via sudo -n (e.g., missing sudoers entry) or fails for a non-transient reason, the user waits two minutes only to get the generic "Suricata could not start." error, which misdiagnoses the actual problem. Distinguish transient connection failures from other failures, or at least include the suricatasc output/stderr in the final error message. 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()) Kill After Child Reaped When Suricata exits during startup, wait_on_suricata_start returns an error after child.try_wait() has already reaped the child, and the caller then invokes kill_suricata(&mut child). Depending on how kill_suricata terminates the process, operating on an already-reaped child can either fail or, on Unix, send a signal to the stored PID which may have been reused by an unrelated process. The window is narrow, but the exit path should skip the kill (or verify liveness) when the child has already been reaped. let suri_pid = match wait_on_suricata_start(&suriconf.socket, &mut child) { Ok(pid) => pid, Err(e) => { kill_suricata(&mut child); panic!("{e}") } };

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Propagate status-check errors instead of panicking This function returns a Result, but both child.try_wait() calls use .expect(), which converts an I/O error into a panic instead of a graceful error. Use map_err to propagate the failure as part of the Result so callers (which already handle errors by killing Suricata) can react properly. src/suricata.rs [328-330] - if let Some(status) = child.try_wait().expect("Unable to get Suricata status.") { + if let Some(status) = child + .try_wait() + .map_err(|e| format!("Unable to get Suricata status: {e}"))? + { return Err(format!("Suricata exited during startup with status: {status}.")); } Suggestion importance[1-10]: 6 __ Why: The try_wait().expect() calls in wait_on_suricata_start do panic on I/O errors despite the function returning a Result, so converting them to map_err is a correct and consistent improvement since callers already kill Suricata on error. Impact is moderate as this only affects rare I/O failure paths.	Low
	Verify socket command output, not just exit status suricatasc can exit with status 0 even when the command did not succeed (e.g., the socket file exists but Suricata is not fully initialized, or sudo -n fails and writes to stderr), and it prints errors to stderr while still returning 0. Inspect output.stdout/output.stderr to confirm the uptime response is valid before treating startup as successful, and include stderr in the failure diagnostics to distinguish "not ready yet" from "suricatasc unavailable". src/suricata.rs [347-358] 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); + let stdout = String::from_utf8_lossy(&output.stdout); + if !stdout.contains("failure") && !stdout.contains("None") { + return Ok(pid); + } } Suggestion importance[1-10]: 4 __ Why: The concern that suricatasc may succeed (exit 0) while the socket is not truly ready is plausible, but the proposed stdout.contains("failure")/"None" heuristic is fragile and somewhat speculative. It's a reasonable robustness consideration with limited, uncertain impact.	Low
General	Increase and configure startup wait timeout The PID-file wait is capped at roughly 20 seconds, which may be too short on slow systems or with large rule sets before Suricata writes its pidfile, causing a spurious "Suricata process not found." failure. Consider making this timeout configurable (similar to preconf_time) or significantly increasing it, and reuse the existing child.try_wait() check so the loop exits as soon as the child dies rather than sleeping the full duration. src/suricata.rs [317-326] - for _ in 0..20 { + for _ in 0..120 { 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" { suri_pid = Some(pid); } } } } Suggestion importance[1-10]: 4 __ Why: Increasing the 20-second pidfile wait improves robustness on slow systems, and making it configurable is a reasonable enhancement. However, part of the suggestion (exiting when the child dies) is already implemented in the loop, so only the timeout increase adds real value.	Low
	Preserve Suricata startup output for diagnostics Redirecting Suricata's stdout to Stdio::null() discards valuable startup diagnostics (rule loading progress, warnings, and fatal errors), which makes debugging startup failures detected by wait_on_suricata_start much harder. Keep stdout piped and spawn a reader thread mirroring the existing stderr handling, or redirect it to a log file under the configured log directory. src/suricata.rs [77-83] let mut child = Command::new("sudo") .arg("-n") .args(args) - .stdout(Stdio::null()) + .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() .expect("Failed to execute process."); + if let Some(stdout) = child.stdout.take() { + let reader = BufReader::new(stdout); + thread::spawn(move || { + for line in reader.lines().flatten() { + println!("{line}"); + } + }); + } + Suggestion importance[1-10]: 3 __ Why: The suggestion directly reverses a deliberate PR change (stdout from piped to null), likely intended to reduce noise or resource use. While retaining startup diagnostics has debugging value, it contradicts the PR's explicit modification and offers only marginal benefit.	Low

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

[KEIAHNY]

All issues have been reviewed. The remaining potential issues were analyzed and determined to be either invalid or unnecessary to address.