Bug/12187 v4 reliably detect Suricata startup

[KEIAHNY]

PR Type

Enhancement, Bug fix

---

Description

• Implement PID file and socket startup check

• Add CLI argument for socket path configuration

• Update Suriconf struct to store socket path

• Remove unreliable process name scanning logic

---

Diagram Walkthrough

flowchart TD
  A["Start"] --> B["Delete PID File"]
  B --> C["Start Suricata"]
  C --> D["Wait for PID File"]
  D --> E["Check Socket via suricatasc"]
  E --> F["Ready"]

Loading

File Walkthrough

	Relevant files
Enhancement	argument.rs Add socket path CLI argument                                                          src/argument.rs Added path_to_socket argument to Commands::Suricata Enables CLI configuration for Unix socket path +4/-0      yaml.rs Add socket field to config                                                              src/yaml.rs Added socket field to Suriconf struct Implemented find_socket parsing helper Updated initialization to handle socket path +12/-0
Bug fix	suricata.rs Refactor startup detection logic                                                  src/suricata.rs Implemented wait_on_suricata_start using PID and socket Added delete_pid_file and set_pid_file helpers Removed check_process_name_for_suricata_main logic Changed stdout to Stdio::null during execution +77/-17
Configuration changes	suriconf.yaml Define default socket path                                                              src/suriconf.yaml Added default socket path configuration Sets /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: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Startup Timeout Too Short get_suricata_pid and wait_on_suricata_start each bound the wait to 10 attempts of 1 second. Suricata only opens its unix command socket after full initialization, which on large rule sets or slow disks can take well over 10 seconds. In that scenario the tool panics with 'Suricata could not start.' even though Suricata would have started successfully moments later, killing the run. Consider making the timeout configurable (e.g., reusing preconf_time) or retrying on the total startup budget rather than a fixed 10 seconds. Impact is limited to environments where startup exceeds ~10 seconds. 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); } } } thread::sleep(Duration::from_millis(1000)); } Err("Suricata process not found.".into()) } fn wait_on_suricata_start(socket: &PathBuf) -> 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..10 { 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)); } Err("Suricata could not start.".into()) } Err(e) => { Err(e) } } } Stale Instance Detection wait_on_suricata_start only checks that suricatasc -c uptime against the configured socket succeeds. If a leftover Suricata instance from a previous crashed run is still bound to that same socket, the check passes even though the newly spawned Suricata may have failed to bind the socket, and the tool proceeds with uptime/PID data from the old instance. The PID-file deletion in delete_pid_file removes the stale pid file but does not stop a stale process. Worth confirming whether killing the previous instance is expected before start. fn wait_on_suricata_start(socket: &PathBuf) -> 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..10 { 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)); } Err("Suricata could not start.".into())

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Pass socket path via the --socket flag suricatasc does not accept the socket path as a positional argument; it must be passed via the --socket flag. As written, the command fails every iteration (or binds to the default socket), so wait_on_suricata_start will report "Suricata could not start" even when Suricata is running. Pass the socket path using --socket. src/suricata.rs [331-338] let output = Command::new("sudo") .arg("-n") .arg("suricatasc") .arg("-c") .arg("uptime") + .arg("--socket") .arg(socket) .status() .map_err(|e| format!("Unable to execute Suricata socket control tool: {e}"))?; Suggestion importance[1-10]: 8 __ Why: suricatasc expects the socket path via the --socket flag, not as a positional argument, so the current command would fail (or use the default socket) and cause wait_on_suricata_start to incorrectly report that Suricata could not start. This is a critical correctness bug in the new readiness check logic.	Medium
	Align Suricata's socket with the readiness check The socket path used for the suricatasc readiness check is read from suriconf.socket, but Suricata itself is never told to use that socket path. If the configured socket differs from the one in Suricata's own configuration, the readiness check will always fail. Add --unix-socket with suriconf.socket to the Suricata command line so both sides agree. src/suricata.rs [234-237] -pub fn set_pid_file(vec_of_sur_cmd: &mut Vec<String>) { +pub fn set_pid_file(vec_of_sur_cmd: &mut Vec<String>, socket: &Path) { vec_of_sur_cmd.push("--pidfile".to_string()); vec_of_sur_cmd.push(PIDFILE.to_string()); + vec_of_sur_cmd.push("--unix-socket".to_string()); + vec_of_sur_cmd.push(socket.display().to_string()); } Suggestion importance[1-10]: 5 __ Why: The readiness check uses suriconf.socket, but Suricata's socket path comes from its own configuration, so a mismatch would make the check always fail. Passing --unix-socket with the same path is a reasonable robustness improvement, though the setup assumes the operator keeps both configs in sync, so the impact is moderate rather than critical.	Low
General	Verify the PID belongs to Suricata Verifying only that /proc/{pid} exists is not sufficient: the PID may belong to an unrelated process (e.g., a pre-existing system Suricata instance or a reused PID), and subsequent functions like get_cores_with_threads will then inspect the wrong process. Validate that the process command name is actually Suricata before returning the PID. src/suricata.rs [315-320] 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); + if let Ok(comm) = fs::read_to_string(format!("/proc/{}/comm", pid)) { + if comm.trim().contains("Suricata") { + return Ok(pid); + } } } } Suggestion importance[1-10]: 4 __ Why: Checking only that /proc/{pid} exists could match an unrelated process due to PID reuse, leading get_cores_with_threads to inspect the wrong process. Since the PR already deletes the stale PID file at startup, the risk is low, making this a minor defensive improvement.	Low
	Signal worker threads before panic exit Before panicking, the kill atomic flag shared with the already-spawned monitor threads is never set, so those threads (and the stderr reader) are left running with the child half-shutdown. Set kill_thread1/kill_thread2 (or the shared kill flag) to true before killing and panicking so all spawned threads terminate cleanly. src/suricata.rs [95-101] let suri_pid = match wait_on_suricata_start(&suriconf.socket) { Ok(pid) => pid, Err(e) => { + kill_thread1.store(true, Ordering::SeqCst); + kill_thread2.store(true, Ordering::SeqCst); kill_suricata(&mut child); panic!("{e}") } }; Suggestion importance[1-10]: 2 __ Why: A panic! on the main thread terminates the entire process, which also kills all spawned threads, so the claim that threads are "left running" is inaccurate and setting the kill flags has essentially no observable effect here. The change is harmless but addresses a non-issue.	Low

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