Repository: NordicHPC/sonar (language: python)

Issue:
Supply chain security / simplify dependencies
This has been on my mind for a while but the recent exploitation of xz makes it a good time to talk about it.  Sonar runs (or will run) on every node on almost every supercomputer in Norway.  It pulls in not too many dependencies but some of them (clap, serde, libc) are quite large and actively maintained and some (subprocess, libc) are expected to do dark things by their nature.

https://news.ycombinator.com/item?id=39902241
https://research.swtch.com/xz-timeline

serde is an interesting case in point because for a while (possibly still but I've not been able to substantiate that) it shipped a precompiled binary in order to speed up compilation time, https://github.com/serde-rs/serde/issues/2538.  With this type of setup, vetting what we pull in and how it works is very hard.

Sonar actually has very simple needs and most of its libraries have been pulled in not because we need the full functionality but only a small part of it.  It may be to our benefit to go over the list of dependencies, remove what we can, and perhaps audit and pin what we can't remove, and define some kind of upgrade protocol.  For example, we have simple needs for csv and json production and for command line parsing and could easily replace those libraries if we wanted to.

I'm uncertain about how worried to be about this.  Sonar *currently* runs with no special privileges and its only superpowers are that (a) it runs regularly and everywhere and (b) if an admin sees it running, she will think it is fine that it's running.  It is otherwise no worse than any user program, indeed user programs whose source and binary we do not control.  On the other hand, being everywhere and running all the time under the radar it is a very convenient stepping stone for a more sophisticated attack leveraging other mechanisms.  And it is not out of the realm of the possible that Sonar will need some special powers eventually, for some types of monitoring.  And if not Sonar, then maybe a program that Sonar runs - same problem.


Discussion / hints:
Great point. Let's go together through list of dependencies and let's check what we use them for and how much work it would be to replace some of them.

Fix patch:
diff --git a/Cargo.toml b/Cargo.toml
index ca62476..d73a460 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,4 +7,4 @@ edition = "2021"
 
 [dependencies]
 libc = "0.2"
-subprocess = "0.2"
+subprocess = { version = "= 0.2.9" }
diff --git a/README.md b/README.md
index 5198c6b..48a551d 100644
--- a/README.md
+++ b/README.md
@@ -434,6 +434,22 @@ of processes.
 
 Optionally, `sonar` will use a lockfile to avoid a pile-up of processes.
 
+## Dependencies and updates
+
+Sonar runs everywhere and all the time, and even though it currently runs without privileges it
+strives to have as few dependencies as possible, so as not to become a target through a supply chain
+attack.  There are some rules:
+
+- It's OK to depend on libc and to incorporate new versions of libc
+- It's better to depend on something from the rust-lang organization than on something else
+- Every dependency needs to be justified
+- Every dependency must have a compatible license
+- Every dependency needs to be vetted as to active development, apparent quality, test cases
+- Every dependency update - even for security issues - is to be considered a code change that needs review
+- Remember that indirect dependencies are dependencies for us, too, and need to be treated the same way
+- If in doubt: copy the parts we need, vet them thoroughly, and maintain them separately
+
+There is a useful discussion of these matters [here](https://research.swtch.com/deps).
 
 ## How we run sonar on a cluster
 
diff --git a/src/amd.rs b/src/amd.rs
index bfaeea8..ec9e4a2 100644
--- a/src/amd.rs
+++ b/src/amd.rs
@@ -76,7 +76,11 @@ pub fn get_amd_information(user_by_pid: &UserTable) -> Result<Vec<gpu::Process>,
 
     match command::safe_command(AMD_CONCISE_COMMAND, AMD_CONCISE_ARGS, TIMEOUT_SECONDS) {
         Ok(concise_raw_text) => {
-            match command::safe_command(AMD_SHOWPIDGPUS_COMMAND, AMD_SHOWPIDGPUS_ARGS, TIMEOUT_SECONDS) {
+            match command::safe_command(
+                AMD_SHOWPIDGPUS_COMMAND,
+                AMD_SHOWPIDGPUS_ARGS,
+                TIMEOUT_SECONDS,
+            ) {
                 Ok(showpidgpus_raw_text) => Ok(extract_amd_information(
                     &concise_raw_text,
                     &showpidgpus_raw_text,
diff --git a/src/command.rs b/src/command.rs
index eab1834..332f106 100644
--- a/src/command.rs
+++ b/src/command.rs
@@ -22,7 +22,11 @@ pub enum CmdError {
 // especially at https://github.com/rust-lang/rust/issues/45572#issuecomment-860134955.  See also
 // https://doc.rust-lang.org/std/process/index.html (second code blob under "Handling I/O").
 
-pub fn safe_command(command: &str, args: &[&str], timeout_seconds: u64) -> Result<String, CmdError> {
+pub fn safe_command(
+    command: &str,
+    args: &[&str],
+    timeout_seconds: u64,
+) -> Result<String, CmdError> {
     let mut p = match Exec::cmd(command)
         .args(args)
         .stdout(Redirection::Pipe)
@@ -168,7 +172,9 @@ fn test_safe_command() {
         Err(_) => assert!(false),
     }
     // This really needs to be the output
-    assert!(safe_command("grep", &["^name =", "Cargo.toml"], 2) == Ok("name = \"sonar\"\n".to_string()));
+    assert!(
+        safe_command("grep", &["^name =", "Cargo.toml"], 2) == Ok("name = \"sonar\"\n".to_string())
+    );
     // Not found
     match safe_command("no-such-command-we-hope", &[], 2) {
         Err(CmdError::CouldNotStart(_)) => {}
diff --git a/src/hostname.rs b/src/hostname.rs
index 9feaaf0..16b696a 100644
--- a/src/hostname.rs
+++ b/src/hostname.rs
@@ -28,24 +28,20 @@ SOFTWARE.
  */
 
 use std::ffi::OsString;
-use std::os::unix::ffi::OsStringExt;
 use std::io;
-use libc;
+use std::os::unix::ffi::OsStringExt;
 
 pub fn get() -> io::Result<OsString> {
     // According to the POSIX specification,
     // host names are limited to `HOST_NAME_MAX` bytes
     //
     // https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html
-    let size =
-        unsafe { libc::sysconf(libc::_SC_HOST_NAME_MAX) as libc::size_t };
+    let size = unsafe { libc::sysconf(libc::_SC_HOST_NAME_MAX) as libc::size_t };
 
     // Stack buffer OK: HOST_NAME_MAX is typically very small (64 on Linux).
     let mut buffer = vec![0u8; size];
 
-    let result = unsafe {
-        libc::gethostname(buffer.as_mut_ptr() as *mut libc::c_char, size)
-    };
+    let result = unsafe { libc::gethostname(buffer.as_mut_ptr() as *mut libc::c_char, size) };
 
     if result != 0 {
         return Err(io::Error::last_os_error());
@@ -61,9 +57,8 @@ fn wrap_buffer(mut bytes: Vec<u8>) -> OsString {
     let end = bytes
         .iter()
         .position(|&byte| byte == 0x00)
-        .unwrap_or_else(|| bytes.len());
+        .unwrap_or(bytes.len());
     bytes.resize(end, 0x00);
 
     OsString::from_vec(bytes)
 }
-
diff --git a/src/interrupt.rs b/src/interrupt.rs
index c400d84..f715676 100644
--- a/src/interrupt.rs
+++ b/src/interrupt.rs
@@ -1,3 +1,6 @@
+#[cfg(debug_assertions)]
+use crate::log;
+
 use std::sync::atomic::{AtomicBool, Ordering};
 
 // Signal handling logic.
@@ -19,15 +22,15 @@ extern "C" fn sonar_signal_handler(_: libc::c_int) {
 
 pub fn handle_interruptions() {
     unsafe {
-        let nomask : libc::sigset_t = std::mem::zeroed();
-        let mut action = libc::sigaction {
+        let nomask: libc::sigset_t = std::mem::zeroed();
+        let action = libc::sigaction {
             sa_sigaction: sonar_signal_handler as usize,
             sa_mask: nomask,
             sa_flags: 0,
             sa_restorer: None,
         };
-        libc::sigaction(libc::SIGTERM, &mut action, std::ptr::null_mut());
-        libc::sigaction(libc::SIGHUP, &mut action, std::ptr::null_mut());
+        libc::sigaction(libc::SIGTERM, &action, std::ptr::null_mut());
+        libc::sigaction(libc::SIGHUP, &action, std::ptr::null_mut());
     }
 }
 
@@ -38,7 +41,8 @@ pub fn is_interrupted() -> bool {
     }
     let flag = INTERRUPTED.load(Ordering::Relaxed);
     if flag {
-        println!("Interrupt flag was set!")
+        // Test cases depend on this exact output.
+        log::info("Interrupt flag was set!")
     }
     flag
 }
diff --git a/src/jobs.rs b/src/jobs.rs
index 76c2bbb..62d70e2 100644
--- a/src/jobs.rs
+++ b/src/jobs.rs
@@ -9,5 +9,6 @@ pub trait JobManager {
     //
     // There's an assumption here that the process map is always the same for all lookups
     // performed on a particular instance of JobManager.
-    fn job_id_from_pid(&mut self, pid: usize, processes: &HashMap<usize, procfs::Process>) -> usize;
+    fn job_id_from_pid(&mut self, pid: usize, processes: &HashMap<usize, procfs::Process>)
+        -> usize;
 }
diff --git a/src/main.rs b/src/main.rs
index 5504e3b..452d4f2 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -17,7 +17,7 @@ mod users;
 mod util;
 
 const TIMEOUT_SECONDS: u64 = 5; // For subprocesses
-const USAGE_ERROR: i32 = 2;     // clap, Python, Go
+const USAGE_ERROR: i32 = 2; // clap, Python, Go
 
 enum Commands {
     /// Take a snapshot of the currently running processes
@@ -115,10 +115,12 @@ fn main() {
 //  - all error reporting is via a generic "usage" message, without specificity as to what was wrong
 
 fn command_line() -> Commands {
-    let mut args = std::env::args();
-    let _executable = args.next();
-    if let Some(command) = args.next() {
-        match command.as_str() {
+    let args = std::env::args().collect::<Vec<String>>();
+    let mut next = 1;
+    if next < args.len() {
+        let command = args[next].as_ref();
+        next += 1;
+        match command {
             "ps" => {
                 let mut batchless = false;
                 let mut rollup = false;
@@ -129,42 +131,43 @@ fn command_line() -> Commands {
                 let mut exclude_users = None;
                 let mut exclude_commands = None;
                 let mut lockdir = None;
-                loop {
-                    if let Some(arg) = args.next() {
-                        match arg.as_str() {
-                            "--batchless" => {
-                                batchless = true;
-                            }
-                            "--rollup" => {
-                                rollup = true;
-                            }
-                            "--exclude-system-jobs" => {
-                                exclude_system_jobs = true;
-                            }
-                            "--exclude-users" => {
-                                (args, exclude_users) = string_value(args);
-                            }
-                            "--exclude-commands" => {
-                                (args, exclude_commands) = string_value(args);
-                            }
-                            "--lockdir" => {
-                                (args, lockdir) = string_value(args);
-                            }
-                            "--min-cpu-percent" => {
-                                (args, min_cpu_percent) = parsed_value::<f64>(args);
-                            }
-                            "--min-mem-percent" => {
-                                (args, min_mem_percent) = parsed_value::<f64>(args);
-                            }
-                            "--min-cpu-time" => {
-                                (args, min_cpu_time) = parsed_value::<usize>(args);
-                            }
-                            _ => {
-                                usage(true);
-                            }
-                        }
+                while next < args.len() {
+                    let arg = args[next].as_ref();
+                    next += 1;
+                    if let Some(new_next) = bool_arg(arg, &args, next, "--batchless") {
+                        (next, batchless) = (new_next, true);
+                    } else if let Some(new_next) = bool_arg(arg, &args, next, "--rollup") {
+                        (next, rollup) = (new_next, true);
+                    } else if let Some(new_next) =
+                        bool_arg(arg, &args, next, "--exclude-system-jobs")
+                    {
+                        (next, exclude_system_jobs) = (new_next, true);
+                    } else if let Some((new_next, value)) =
+                        string_arg(arg, &args, next, "--exclude-users")
+                    {
+                        (next, exclude_users) = (new_next, Some(value));
+                    } else if let Some((new_next, value)) =
+                        string_arg(arg, &args, next, "--exclude-commands")
+                    {
+                        (next, exclude_commands) = (new_next, Some(value));
+                    } else if let Some((new_next, value)) =
+                        string_arg(arg, &args, next, "--lockdir")
+                    {
+                        (next, lockdir) = (new_next, Some(value));
+                    } else if let Some((new_next, value)) =
+                        numeric_arg::<f64>(arg, &args, next, "--min-cpu-percent")
+                    {
+                        (next, min_cpu_percent) = (new_next, Some(value));
+                    } else if let Some((new_next, value)) =
+                        numeric_arg::<f64>(arg, &args, next, "--min-mem-percent")
+                    {
+                        (next, min_mem_percent) = (new_next, Some(value));
+                    } else if let Some((new_next, value)) =
+                        numeric_arg::<usize>(arg, &args, next, "--min-cpu-time")
+                    {
+                        (next, min_cpu_time) = (new_next, Some(value));
                     } else {
-                        break;
+                        usage(true);
                     }
                 }
 
@@ -178,7 +181,8 @@ fn command_line() -> Commands {
                     eprintln!("--rollup and --batchless are incompatible");
                     std::process::exit(USAGE_ERROR);
                 }
-                return Commands::PS {
+
+                Commands::PS {
                     batchless,
                     rollup,
                     min_cpu_percent,
@@ -188,9 +192,9 @@ fn command_line() -> Commands {
                     exclude_users,
                     exclude_commands,
                     lockdir,
-                };
+                }
             }
-            "sysinfo" => return Commands::Sysinfo {},
+            "sysinfo" => Commands::Sysinfo {},
             "help" => {
                 usage(false);
             }
@@ -203,24 +207,47 @@ fn command_line() -> Commands {
     }
 }
 
-fn string_value(mut args: std::env::Args) -> (std::env::Args, Option<String>) {
-    if let Some(val) = args.next() {
-        (args, Some(val))
+fn bool_arg(arg: &str, _args: &[String], next: usize, opt_name: &str) -> Option<usize> {
+    if arg == opt_name {
+        Some(next)
     } else {
-        usage(true);
+        None
+    }
+}
+
+fn string_arg(arg: &str, args: &[String], next: usize, opt_name: &str) -> Option<(usize, String)> {
+    if arg == opt_name {
+        if next < args.len() {
+            Some((next + 1, args[next].to_string()))
+        } else {
+            None
+        }
+    } else if let Some((first, rest)) = arg.split_once('=') {
+        if first == opt_name {
+            Some((next, rest.to_string()))
+        } else {
+            None
+        }
+    } else {
+        None
     }
 }
 
-fn parsed_value<T: std::str::FromStr>(mut args: std::env::Args) -> (std::env::Args, Option<T>) {
-    if let Some(val) = args.next() {
-        match val.parse::<T>() {
-            Ok(value) => (args, Some(value)),
+fn numeric_arg<T: std::str::FromStr>(
+    arg: &str,
+    args: &[String],
+    next: usize,
+    opt_name: &str,
+) -> Option<(usize, T)> {
+    if let Some((next, strval)) = string_arg(arg, args, next, opt_name) {
+        match strval.parse::<T>() {
+            Ok(value) => Some((next, value)),
             _ => {
                 usage(true);
             }
         }
     } else {
-        usage(true);
+        None
     }
 }
 
diff --git a/src/nvidia.rs b/src/nvidia.rs
index 83ae704..2c13cde 100644
--- a/src/nvidia.rs
+++ b/src/nvidia.rs
@@ -238,8 +238,10 @@ fn parse_pmon_output(raw_text: &str, user_by_pid: &UserTable) -> Result<Vec<gpu:
 
 const NVIDIA_QUERY_COMMAND: &str = "nvidia-smi";
 
-const NVIDIA_QUERY_ARGS: &[&str] =
-    &["--query-compute-apps=pid,used_memory", "--format=csv,noheader,nounits"];
+const NVIDIA_QUERY_ARGS: &[&str] = &[
+    "--query-compute-apps=pid,used_memory",
+    "--format=csv,noheader,nounits",
+];
 
 // Same signature as extract_nvidia_pmon_processes(), q.v. but user is always "_zombie_<PID>" and
 // command is always "_unknown_".  Only pids not in user_by_pid are returned.
diff --git a/src/procfs.rs b/src/procfs.rs
index 9975d57..cfc56b0 100644
--- a/src/procfs.rs
+++ b/src/procfs.rs
@@ -350,20 +350,23 @@ pub fn get_process_information(
             99.9,
         );
 
-        result.insert(pid, Process {
+        result.insert(
             pid,
-            ppid,
-            pgrp,
-            uid: uid as usize,
-            user: user_table.lookup(fs, uid),
-            cpu_pct: pcpu_formatted,
-            mem_pct: pmem,
-            cputime_sec,
-            mem_size_kib: size_kib,
-            rssanon_kib,
-            command: comm,
-            has_children: false,
-        });
+            Process {
+                pid,
+                ppid,
+                pgrp,
+                uid: uid as usize,
+                user: user_table.lookup(fs, uid),
+                cpu_pct: pcpu_formatted,
+                mem_pct: pmem,
+                cputime_sec,
+                mem_size_kib: size_kib,
+                rssanon_kib,
+                command: comm,
+                has_children: false,
+            },
+        );
         ppids.insert(ppid);
     }
 
diff --git a/src/procfsapi.rs b/src/procfsapi.rs
index 287abfb..2714a43 100644
--- a/src/procfsapi.rs
+++ b/src/procfsapi.rs
@@ -1,8 +1,6 @@
 // This creates a API by which procfs can access the underlying computing system, allowing the
 // system to be virtualized.  In turn, that allows sensible test cases to be written.
 
-extern crate libc;
-
 use crate::users::get_user_by_uid;
 
 use std::fs;
@@ -58,14 +56,12 @@ impl ProcfsAPI for RealFS {
     fn read_proc_pids(&self) -> Result<Vec<(usize, u32)>, String> {
         let mut pids = vec![];
         if let Ok(dir) = fs::read_dir("/proc") {
-            for dirent in dir {
-                if let Ok(dirent) = dirent {
-                    if let Ok(meta) = dirent.metadata() {
-                        let uid = meta.st_uid();
-                        if let Some(name) = dirent.path().file_name() {
-                            if let Ok(pid) = name.to_string_lossy().parse::<usize>() {
-                                pids.push((pid, uid));
-                            }
+            for dirent in dir.flatten() {
+                if let Ok(meta) = dirent.metadata() {
+                    let uid = meta.st_uid();
+                    if let Some(name) = dirent.path().file_name() {
+                        if let Ok(pid) = name.to_string_lossy().parse::<usize>() {
+                            pids.push((pid, uid));
                         }
                     }
                 }
diff --git a/src/ps.rs b/src/ps.rs
index 2a5f72f..26482e2 100644
--- a/src/ps.rs
+++ b/src/ps.rs
@@ -9,7 +9,7 @@ use crate::log;
 use crate::nvidia;
 use crate::procfs;
 use crate::procfsapi;
-use crate::util::{csv_quote,three_places};
+use crate::util::{csv_quote, three_places};
 
 use std::collections::{HashMap, HashSet};
 use std::io::{self, Result, Write};
@@ -253,6 +253,7 @@ pub fn create_snapshot(jobs: &mut dyn jobs::JobManager, opts: &PsOptions, timest
         }
 
         if skip {
+            // Test cases depend on this exact message.
             log::info("Lockfile present, exiting");
         }
         if failed {
@@ -263,11 +264,7 @@ pub fn create_snapshot(jobs: &mut dyn jobs::JobManager, opts: &PsOptions, timest
     }
 }
 
-fn do_create_snapshot(
-    jobs: &mut dyn jobs::JobManager,
-    opts: &PsOptions,
-    timestamp: &str,
-) {
+fn do_create_snapshot(jobs: &mut dyn jobs::JobManager, opts: &PsOptions, timestamp: &str) {
     let no_gpus = empty_gpuset();
     let mut proc_by_pid = ProcTable::new();
 
@@ -300,13 +297,13 @@ fn do_create_snapshot(
 
     // The table of users is needed to get GPU information, see comments at UserTable.
     let mut user_by_pid = UserTable::new();
-    for (_, proc) in pprocinfo_output {
+    for proc in pprocinfo_output.values() {
         user_by_pid.insert(proc.pid, (&proc.user, proc.uid));
     }
 
     let mut lookup_job_by_pid = |pid: Pid| jobs.job_id_from_pid(pid, pprocinfo_output);
 
-    for (_, proc) in pprocinfo_output {
+    for proc in pprocinfo_output.values() {
         add_proc_info(
             &mut proc_by_pid,
             &mut lookup_job_by_pid,
@@ -347,12 +344,11 @@ fn do_create_snapshot(
         }
         Ok(ref nvidia_output) => {
             for proc in nvidia_output {
-                let (ppid, has_children) =
-                    if let Some(process) = pprocinfo_output.get(&proc.pid) {
-                        (process.ppid, process.has_children)
-                    } else {
-                        (1, true)
-                    };
+                let (ppid, has_children) = if let Some(process) = pprocinfo_output.get(&proc.pid) {
+                    (process.ppid, process.has_children)
+                } else {
+                    (1, true)
+                };
                 add_proc_info(
                     &mut proc_by_pid,
                     &mut lookup_job_by_pid,
@@ -390,12 +386,11 @@ fn do_create_snapshot(
         }
         Ok(ref amd_output) => {
             for proc in amd_output {
-                let (ppid, has_children) =
-                    if let Some(process) = pprocinfo_output.get(&proc.pid) {
-                        (process.ppid, process.has_children)
-                    } else {
-                        (1, true)
-                    };
+                let (ppid, has_children) = if let Some(process) = pprocinfo_output.get(&proc.pid) {
+                    (process.ppid, process.has_children)
+                } else {
+                    (1, true)
+                };
                 add_proc_info(
                     &mut proc_by_pid,
                     &mut lookup_job_by_pid,
@@ -714,7 +709,7 @@ fn print_record(
     }
     s += "\n";
 
-    writer.write(s.as_bytes())?;
+    let _ = writer.write(s.as_bytes())?;
 
     Ok(true)
 }
diff --git a/src/slurm.rs b/src/slurm.rs
index c542117..85e33d1 100644
--- a/src/slurm.rs
+++ b/src/slurm.rs
@@ -3,14 +3,18 @@
 use crate::jobs;
 use crate::procfs;
 
+use std::collections::HashMap;
 use std::fs::File;
 use std::io::{BufRead, BufReader};
-use std::collections::HashMap;
 
 pub struct SlurmJobManager {}
 
 impl jobs::JobManager for SlurmJobManager {
-    fn job_id_from_pid(&mut self, pid: usize, _processes: &HashMap<usize, procfs::Process>) -> usize {
+    fn job_id_from_pid(
+        &mut self,
+        pid: usize,
+        _processes: &HashMap<usize, procfs::Process>,
+    ) -> usize {
         let slurm_job_id = get_slurm_job_id(pid).unwrap_or_default();
         slurm_job_id.trim().parse::<usize>().unwrap_or_default()
     }
diff --git a/src/time.rs b/src/time.rs
index ce6c54e..b5bc2df 100644
--- a/src/time.rs
+++ b/src/time.rs
@@ -1,4 +1,3 @@
-use libc;
 use std::ffi::CStr;
 
 // Get current time as an ISO time stamp: yyyy-mm-ddThh:mm:ss+hhmm
@@ -24,12 +23,12 @@ pub fn now_iso8601() -> String {
         tm_gmtoff: 0,
         tm_zone: std::ptr::null(),
     };
-    const SIZE: usize = 32;     // We need 25 unless something is greatly off
+    const SIZE: usize = 32; // We need 25 unless something is greatly off
     let mut buffer = vec![0i8; SIZE];
     unsafe {
         let t = libc::time(std::ptr::null_mut());
 
-        if libc::localtime_r(&t, &mut timebuf) == std::ptr::null_mut() {
+        if libc::localtime_r(&t, &mut timebuf).is_null() {
             panic!("localtime_r");
         }
 
@@ -38,11 +37,16 @@ pub fn now_iso8601() -> String {
             buffer.as_mut_ptr(),
             SIZE,
             CStr::from_bytes_with_nul_unchecked(b"%FT%T%z\0").as_ptr(),
-            &timebuf) == 0 {
+            &timebuf,
+        ) == 0
+        {
             panic!("strftime");
         }
 
-        return CStr::from_ptr(buffer.as_ptr()).to_str().unwrap().to_string();
+        return CStr::from_ptr(buffer.as_ptr())
+            .to_str()
+            .unwrap()
+            .to_string();
     }
 }
 


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index fa34cf1..8d3ab56 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -14,7 +14,20 @@ jobs:
     steps:
     - name: Checkout
       uses: actions/checkout@v2
+
     - name: Build
       run: cargo build --verbose
+
     - name: Test
       run: cargo test --verbose
+
+      # one of the shell tests below needs jq
+    - name: Install jq
+      run: |
+        sudo apt-get update
+        sudo apt-get install -y jq
+
+    - name: Shell tests
+      run: |
+        cd tests
+        ./run_tests.sh
diff --git a/tests/command-line.sh b/tests/command-line.sh
new file mode 100755
index 0000000..bafbb10
--- /dev/null
+++ b/tests/command-line.sh
@@ -0,0 +1,64 @@
+#!/usr/bin/env bash
+#
+# Check that command line parsing is somewhat sane.
+
+set -e
+( cd ..; cargo build )
+
+# Allow both forms of argument syntax
+../target/debug/sonar ps --exclude-users=root,$LOGNAME > /dev/null
+../target/debug/sonar ps --exclude-users root,$LOGNAME > /dev/null
+
+# Test all arguments in combination with --batchless
+../target/debug/sonar ps \
+                      --batchless \
+                      --min-cpu-percent 0.5 \
+                      --min-mem-percent 1.8 \
+                      --min-cpu-time 10 \
+                      --exclude-system-jobs \
+                      --exclude-users root \
+                      --exclude-commands emacs \
+                      --lockdir . \
+                      > /dev/null
+
+# Test all arguments in combination with --rollup
+../target/debug/sonar ps \
+                      --rollup \
+                      --min-cpu-percent 0.5 \
+                      --min-mem-percent 1.8 \
+                      --min-cpu-time 10 \
+                      --exclude-system-jobs \
+                      --exclude-users root \
+                      --exclude-commands emacs \
+                      --lockdir . \
+                      > /dev/null
+
+# Signal error with code 2 for unknown arguments
+set +e
+output=$(../target/debug/sonar ps --zappa 2>&1)
+exitcode=$?
+set -e
+if [[ $exitcode != 2 ]]; then
+    echo "Failed to reject unknown argument"
+    exit 1
+fi
+
+# Signal error with code 2 for invalid arguments: missing string
+set +e
+output=$(../target/debug/sonar ps --lockdir 2>&1)
+exitcode=$?
+set -e
+if [[ $exitcode != 2 ]]; then
+    echo "Lockdir should require an argument value"
+    exit 1
+fi
+
+# Signal error with code 2 for invalid arguments: bad number
+set +e
+output=$(../target/debug/sonar ps --min-cpu-time 7hello 2>&1)
+exitcode=$?
+set -e
+if [[ $exitcode != 2 ]]; then
+    echo "min-cpu-time should require an integer argument value"
+    exit 1
+fi
diff --git a/tests/exclude-commands.sh b/tests/exclude-commands.sh
new file mode 100755
index 0000000..8490fd5
--- /dev/null
+++ b/tests/exclude-commands.sh
@@ -0,0 +1,17 @@
+#!/usr/bin/env bash
+#
+# Test that the --exclude-commands switch works.
+
+set -e
+( cd .. ; cargo build )
+numbad=$(../target/debug/sonar ps --exclude-commands bash,sh,zsh,csh,ksh,tcsh,kworker | \
+    awk "
+/,cmd=kworker/ { print }
+/,cmd=(ba|z|c|k|tc|)sh/ { print }
+" | \
+    wc -l)
+if [[ $numbad -ne 0 ]]; then
+    echo "Command filtering did not work"
+    exit 1
+fi
+
diff --git a/tests/exclude-system-jobs.sh b/tests/exclude-system-jobs.sh
new file mode 100755
index 0000000..f494336
--- /dev/null
+++ b/tests/exclude-system-jobs.sh
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+#
+# Test that the --exclude-system-jobs switch works.  System jobs have uid < 1000.  For each user
+# name UNAME, run `getent passwd $UNAME` and then extract the third field from the colon-separated
+# list to get the uid, then collect the uids that are < 1000 - these are wrong.
+
+set -e
+( cd .. ; cargo build )
+numbad=$(../target/debug/sonar ps --exclude-system-jobs | \
+             awk '
+{
+    s=substr($0, index($0, ",user=")+6)
+    s=substr(s, 0, index(s, ",")-1)
+    uids[s] = 1
+}
+END {
+    s = ""
+    for ( uid in uids ) {
+        s = s " " uid
+    }
+    system("getent passwd " s)
+}
+' | \
+             awk -F: '{ if (strtonum($3) < 1000) { print $3 } }' | \
+             wc -l )
+if [[ $numbad -ne 0 ]]; then
+    echo $numbad
+    echo "System jobs filtering did not work"
+    exit 1
+fi
+
diff --git a/tests/exclude-users.sh b/tests/exclude-users.sh
new file mode 100755
index 0000000..fc7014d
--- /dev/null
+++ b/tests/exclude-users.sh
@@ -0,0 +1,17 @@
+#!/usr/bin/env bash
+#
+# Test that the --exclude-users switch works.
+
+set -e
+( cd .. ; cargo build )
+numbad=$(../target/debug/sonar ps --exclude-users root,root,root,$LOGNAME | \
+    awk "
+/,user=root,/ { print }
+/,user=$LOGNAME,/ { print }
+" | \
+    wc -l)
+if [[ $numbad -ne 0 ]]; then
+    echo "User name filtering did not work"
+    exit 1
+fi
+
diff --git a/tests/hostname.sh b/tests/hostname.sh
new file mode 100755
index 0000000..b3e68cb
--- /dev/null
+++ b/tests/hostname.sh
@@ -0,0 +1,10 @@
+#!/usr/bin/env bash
+#
+# Check that sonar reports the correct hostname
+
+set -e
+( cd ..; cargo build )
+if [[ $(../target/debug/sonar ps | head -n 1 | grep ",host=$(hostname)," | wc -l) == 0 ]]; then
+    echo "Wrong hostname??"
+    exit 1
+fi
diff --git a/tests/interrupt.sh b/tests/interrupt.sh
new file mode 100755
index 0000000..0e6b136
--- /dev/null
+++ b/tests/interrupt.sh
@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+#
+# Test the interrupt logic in sonar.  A TERM or HUP signal can be sent and the process will exit in
+# an orderly way with a message on stderr.
+
+set -e
+echo "This takes about 15s"
+( cd .. ; cargo build )
+rm -f interrupt.output.txt
+SONARTEST_WAIT_INTERRUPT=1 ../target/debug/sonar ps 2> interrupt.output.txt &
+bgpid=$!
+sleep 3
+kill -TERM $bgpid
+sleep 10
+if [[ $(cat interrupt.output.txt) != 'Info: Interrupt flag was set!' ]]; then
+    echo "Unexpected output!"
+    exit 1
+fi
+rm -f interrupt.output.txt
+
diff --git a/tests/lockfile.sh b/tests/lockfile.sh
new file mode 100755
index 0000000..6d5c4b7
--- /dev/null
+++ b/tests/lockfile.sh
@@ -0,0 +1,24 @@
+#!/usr/bin/env bash
+#
+# Test the lock file logic in sonar.  Sonar creates a lock file when it runs; a subsequent run that
+# starts while the lock file exists will terminate immediately with a log message.
+
+set -e
+logfile=lockfile.output.txt
+
+echo "This takes about 15s"
+( cd .. ; cargo build )
+rm -f $logfile sonar-lock.*
+SONARTEST_WAIT_LOCKFILE=1 ../target/debug/sonar ps --lockdir . > /dev/null &
+bgpid=$!
+# Wait for the first process to get going
+sleep 3
+../target/debug/sonar ps --lockdir . 2> $logfile
+if [[ $(cat $logfile) != 'Info: Lockfile present, exiting' ]]; then
+    echo "Unexpected output!"
+    exit 1
+fi
+# Wait for the first process to exit
+sleep 10
+# Do not delete the lockfile here, that should be handled by the first sonar process
+rm -f $logfile
diff --git a/tests/min-cpu-time.sh b/tests/min-cpu-time.sh
new file mode 100755
index 0000000..3b1b7ac
--- /dev/null
+++ b/tests/min-cpu-time.sh
@@ -0,0 +1,22 @@
+#!/usr/bin/env bash
+#
+# Test that the --min-cpu-time switch works.
+
+set -e
+( cd .. ; cargo build )
+numbad=$(../target/debug/sonar ps --min-cpu-time 5 | \
+             awk '
+{
+    s=substr($0, index($0, ",cputime_sec=")+13)
+    # this field is frequently last so no guarantee there is a trailing comma
+    ix = index(s, ",")
+    if (ix > 0)
+        s=substr(s, 0, ix-1)
+    if (strtonum(s) < 5)
+        print($0)
+}' | \
+             wc -l )
+if [[ $numbad -ne 0 ]]; then
+    echo "CPU time filtering did not work"
+    exit 1
+fi
diff --git a/tests/ps-syntax.sh b/tests/ps-syntax.sh
new file mode 100755
index 0000000..85260d6
--- /dev/null
+++ b/tests/ps-syntax.sh
@@ -0,0 +1,40 @@
+#!/usr/bin/env bash
+#
+# Check that `sonar ps` produces some sane output.
+#
+# At the moment, all we do is:
+#  - check that at least one line is produced
+#  - the line starts with `v=` with a sensible version syntax
+#  - there is a `host=$HOSTNAME` field
+#  - there is a `user=` field with a plausible string
+#  - there is a `cmd=` field
+#
+# We don't have the infra at the moment to check the CSV output (cf sysinfo-syntax.sh where we use
+# the jq utility), plus CSV is so flexible that it's sort of hard to check it.
+
+set -e
+( cd .. ; cargo build )
+output=$(../target/debug/sonar ps)
+count=$(wc -l <<< $output)
+if [[ $count -le 0 ]]; then
+    echo "Must have some number of output lines"
+    exit 1
+fi
+l=$(head -n 1 <<< $output)
+if [[ !( $l =~ ^v=[0-9]+\.[0-9]+\.[0-9], ) ]]; then
+    echo "Version missing"
+    exit 1
+fi
+if [[ !( $l =~ ,user=[a-z0-9]+, ) ]]; then
+    echo "User missing"
+    exit 1
+fi
+# The command may be quoted so match only the beginning
+if [[ !( $l =~ ,\"?cmd= ) ]]; then
+    echo "Cmd missing"
+    exit 1
+fi
+if [[ !( $l =~ ,host=$HOSTNAME, ) ]]; then
+    echo "Host missing"
+    exit 1
+fi
diff --git a/tests/rollup.sh b/tests/rollup.sh
index 7733444..3a82822 100755
--- a/tests/rollup.sh
+++ b/tests/rollup.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash
 #
 # Test these aspects of the process rollup algorithm:
 #  - only leaf processes are rolled up
diff --git a/tests/rollup2.sh b/tests/rollup2.sh
index 87111dc..f795923 100755
--- a/tests/rollup2.sh
+++ b/tests/rollup2.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash
 #
 # Test these aspects of the process rollup algorithm:
 #  - only siblings with the same name are rolled up
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
new file mode 100755
index 0000000..8433d43
--- /dev/null
+++ b/tests/run_tests.sh
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+#
+# Primitive test runner.
+
+set -e
+
+# sysinfo-syntax.sh requires jq
+# check whether jq is available and exit if not
+if ! command -v jq &> /dev/null; then
+    echo "ERROR: jq is required for sysinfo-syntax.sh"
+    exit 1
+fi
+
+# keep tests alphabetical
+# later we could just iterate over all scripts that end with .sh
+# and are not this script
+for test in command-line \
+                exclude-commands \
+                exclude-system-jobs \
+                exclude-users \
+                hostname \
+                interrupt \
+                lockfile \
+                min-cpu-time \
+                ps-syntax \
+                sysinfo-syntax \
+                user \
+            ; do
+    echo $test
+    ./$test.sh
+done
+
+echo "No errors"
diff --git a/tests/sysinfo-syntax.sh b/tests/sysinfo-syntax.sh
index 2769281..4727d60 100755
--- a/tests/sysinfo-syntax.sh
+++ b/tests/sysinfo-syntax.sh
@@ -1,6 +1,6 @@
-#!/bin/bash
+#!/usr/bin/env bash
 #
-# Check that sysinfo produces properly formatted JSON.
+# Check that `sonar sysinfo` produces properly formatted JSON.
 # Requirement: the `jq` utility.
 
 set -e
diff --git a/tests/user.sh b/tests/user.sh
new file mode 100755
index 0000000..5c09d0e
--- /dev/null
+++ b/tests/user.sh
@@ -0,0 +1,11 @@
+#!/usr/bin/env bash
+#
+# Check that sonar can look up users.  There will be at least one process for the user: sonar.
+
+set -e
+( cd ..; cargo build )
+if [[ $(../target/debug/sonar ps | grep ",user=$USER," | wc -l) == 0 ]]; then
+    echo "User name lookup fails??"
+    exit 1
+fi
+