diff --git a/crates/houndd/src/persistence.rs b/crates/houndd/src/persistence.rs index 74055f2..2dae0d0 100644 --- a/crates/houndd/src/persistence.rs +++ b/crates/houndd/src/persistence.rs @@ -475,12 +475,39 @@ mod tests { // Read-only mode must not have side effects — someone running a // check should not silently accept whatever is currently installed // as normal. + // + // The env lock is load-bearing, not decoration. baseline_path() + // reads XDG_DATA_HOME, and the quarantine tests reassign it; two + // scans either side of that reassignment look at different files + // and disagree about first_run. That made this test flaky, which + // in a security product is worse than a test that simply fails. + let data = std::env::temp_dir().join(format!("hound-pscan-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&data); + std::fs::create_dir_all(&data).unwrap(); + let _env_guard = crate::test_util::locked(); + std::env::set_var("XDG_DATA_HOME", &data); + let scan_a = scan(false); assert!(scan_a.total > 0); + assert!(scan_a.first_run, "an isolated data dir has no baseline yet"); + assert!( + !baseline_path().exists(), + "a read-only scan must not write a baseline" + ); + let scan_b = scan(false); assert_eq!( scan_a.first_run, scan_b.first_run, "a read-only scan must not change what the next one sees" ); + + // And an explicit accept does write one. + let scan_c = scan(true); + assert!(scan_c.first_run); + assert!(baseline_path().exists(), "--accept must record the baseline"); + assert!(!scan(false).first_run, "and the next scan compares against it"); + + std::env::remove_var("XDG_DATA_HOME"); + let _ = std::fs::remove_dir_all(&data); } } diff --git a/crates/houndd/src/rootkit.rs b/crates/houndd/src/rootkit.rs index b557afc..d51f017 100644 --- a/crates/houndd/src/rootkit.rs +++ b/crates/houndd/src/rootkit.rs @@ -477,44 +477,47 @@ mod tests { // any thread id. A multi-threaded process therefore has ids that // answer kill and are absent from a /proc listing — and reporting // those as hidden produced dozens of criticals on a healthy laptop. + // + // Each thread reports its OWN tid rather than the test reading + // /proc/self/task afterwards. Reading the task list and the /proc + // snapshot at different moments reintroduces exactly the race this + // whole check exists to avoid — the test harness starts and stops + // threads for other tests throughout, so one captured in the first + // read may be gone by the second. Threads that report themselves + // and then wait are alive across the entire window by construction. let stop = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let reported: std::sync::Arc>> = + std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let handles: Vec<_> = (0..8) .map(|_| { let s = std::sync::Arc::clone(&stop); + let r = std::sync::Arc::clone(&reported); std::thread::spawn(move || { + // SAFETY: gettid takes no arguments and cannot fail. + let tid = unsafe { libc::syscall(libc::SYS_gettid) } as u32; + r.lock().expect("tid list poisoned").push(tid); while !s.load(std::sync::atomic::Ordering::Relaxed) { std::thread::sleep(std::time::Duration::from_millis(5)); } }) }) .collect(); - std::thread::sleep(std::time::Duration::from_millis(50)); + // Wait until every thread has reported and is therefore parked. + let mut waited = 0; + while reported.lock().unwrap().len() < 8 && waited < 200 { + std::thread::sleep(std::time::Duration::from_millis(5)); + waited += 1; + } + let mine: Vec = reported.lock().unwrap().clone(); + + // Snapshot while all eight are demonstrably alive. let tids = proc_tids(); let pids = proc_pids(); - - // Our own threads: real task ids that answer kill() and do NOT - // appear in a /proc listing. Every one must be accounted for by - // proc_tids, or it becomes a critical finding on a clean machine. - // - // Everything is measured while the threads are still alive and only - // asserted afterwards — checking a thread's existence after joining - // it tests nothing except that join() works. - let mine: Vec = std::fs::read_dir(format!("/proc/{}/task", std::process::id())) - .unwrap() - .flatten() - .filter_map(|e| e.file_name().to_str().and_then(|s| s.parse::().ok())) - .collect(); let observed: Vec<(u32, bool, bool, bool)> = mine .iter() - .map(|tid| { - ( - *tid, - pid_exists(*tid), - tids.contains(tid), - pids.contains(tid), - ) - }) + .map(|tid| (*tid, pid_exists(*tid), tids.contains(tid), pids.contains(tid))) .collect(); stop.store(true, std::sync::atomic::Ordering::Relaxed); @@ -522,25 +525,23 @@ mod tests { let _ = h.join(); } + assert_eq!(mine.len(), 8, "all eight threads should have reported"); assert!( tids.len() > pids.len(), "this process alone has 8 extra threads, so tids must exceed pids" ); - assert!(mine.len() >= 9, "expected the leader plus 8 threads, got {}", mine.len()); for (tid, exists, in_tids, in_pids) in observed { - assert!(exists, "thread {tid} was alive and must answer kill()"); + assert!(exists, "thread {tid} was parked and must answer kill()"); assert!( in_tids, "thread {tid} answers kill() but proc_tids missed it — it would be \ reported as a hidden process" ); - if tid != std::process::id() { - assert!( - !in_pids, - "thread {tid} should not be a top-level /proc entry; that is exactly \ - why proc_pids alone is insufficient" - ); - } + assert!( + !in_pids, + "thread {tid} is not a top-level /proc entry; that is exactly why \ + proc_pids alone is insufficient" + ); } }