Update clap to remove suppressions (#2856)

A Friday afternoon jaunt.

To be merged after #2855; does the remaining work to close #2277 (and closes off some things that Dependabot was trying to upgrade).

Tested by manually running the commands; we don't have good coverage for this kind of stuff. OTOH, most of these commands are for the experimental local fuzzing mode, which is not fully supported yet. I did specifically test the `onefuzz-task managed` command which is the one used in production.

## Details

- Bump `clap` to 4.1.6
  - Remove `structopt` as this is subsumed by clap now
- Bump `envlogger` to 0.10 (removes problematic dependency)
- Set `default-features=false` on `proc-maps` (removes a feature which is only needed to support FreeBSD), and bump it to 0.3

The main changes migrating `clap` are:

- `value_t!` is gone; now use `matches.get_one::<T>`. If `T` is not `String` then a parser must have been registered on the `Arg` when it was created, with `arg.value_parser(value_parser!(T))`.
- `Command::with_name` and `Arg::with_name` are now called `new`.
- `Command` and `Subcommand` were unified, and `App` is removed.
- `arg.takes_value(true)` is gone; it is the default. For flags use `arg.action(ArgAction::SetTrue)` and then retrieve the flag value with `matches.get_flag`.

This code would be simplified a lot by using the `clap::Parser` on structs, but that requires reworking the code significantly as we cannot dynamically add/remove arguments the way that this is currently done.

## Also found

Found one bug while manually testing the `onefuzz-task local` commands; see comment below.

src/agent/dynamic-library/Cargo.toml

@@ -6,9 +6,9 @@ license = "MIT"
 
 [dependencies]
 anyhow = "1.0"
+clap = { version = "4.1.6", features = ["derive"] }
 lazy_static = "1.4"
 regex = "1.6"
-structopt = "0.3"
 thiserror = "1.0"
 
 [target.'cfg(windows)'.dependencies]

src/agent/dynamic-library/src/bin/dynamic-library.rs

@@ -4,22 +4,22 @@
 use std::process::{Command, Stdio};
 
 use anyhow::Result;
-use structopt::StructOpt;
+use clap::Parser;
 
-#[derive(Debug, StructOpt)]
+#[derive(Parser, Debug)]
 struct Opt {
-    #[structopt(min_values = 1)]
+    #[arg(required = true, num_args = 1..)]
     argv: Vec<String>,
 
-    #[structopt(short, long)]
+    #[arg(short, long)]
     quiet: bool,
 
-    #[structopt(short, long)]
+    #[arg(short, long)]
     ld_library_path: Option<String>,
 }
 
 fn main() -> Result<()> {
-    let opt = Opt::from_args();
+    let opt = Opt::parse();
 
     let exe = &opt.argv[0];
     let mut cmd = Command::new(exe);

src/agent/onefuzz-agent/Cargo.toml

@@ -10,7 +10,7 @@ license = "MIT"
 anyhow = { version = "1.0", features = ["backtrace"] }
 async-trait = "0.1"
 downcast-rs = "1.2"
-env_logger = "0.9"
+env_logger = "0.10"
 futures = "0.3"
 log = "0.4"
 onefuzz = { path = "../onefuzz" }
@@ -25,7 +25,7 @@ storage-queue = { path = "../storage-queue" }
 tokio = { version = "1.24", features = ["full"] }
 url = { version = "2.3", features = ["serde"] }
 uuid = { version = "0.8", features = ["serde", "v4"] }
-clap = { version = "3.2.4", features = ["derive", "cargo"] }
+clap = { version = "4", features = ["derive", "cargo"] }
 reqwest-retry = { path = "../reqwest-retry" }
 onefuzz-telemetry = { path = "../onefuzz-telemetry" }
 backtrace = "0.3"

src/agent/onefuzz-agent/src/main.rs

@@ -54,20 +54,21 @@ enum Opt {
 
 #[derive(Parser, Debug)]
 struct RunOpt {
-    #[clap(short, long = "--config", parse(from_os_str))]
+    #[arg(short, long = "config")]
     config_path: Option<PathBuf>,
+
     /// re-executes as a child process, recording stdout/stderr to files in
     /// the specified directory
-    #[clap(short, long = "--redirect-output", parse(from_os_str))]
+    #[arg(short, long = "redirect-output")]
     redirect_output: Option<PathBuf>,
 
-    #[clap(long = "--machine_id")]
+    #[arg(long = "machine_id")]
     machine_id: Option<Uuid>,
 
-    #[clap(long = "--machine_name")]
+    #[arg(long = "machine_name")]
     machine_name: Option<String>,
 
-    #[clap(long = "--reset_lock", takes_value = false, action = ArgAction::SetTrue )]
+    #[arg(long = "reset_lock", action = ArgAction::SetTrue )]
     reset_node_lock: bool,
 }
 

src/agent/onefuzz-task/Cargo.toml

@@ -15,11 +15,11 @@ arraydeque = "0.5"
 async-trait = "0.1"
 atexit = { path = "../atexit" }
 backoff = { version = "0.4", features = ["tokio"] }
-clap = "2.34"
+clap = { version = "4", features = ["cargo", "string"] }
 cobertura = { path = "../cobertura" }
 coverage = { path = "../coverage" }
 crossterm = "0.22"
-env_logger = "0.9"
+env_logger = "0.10"
 flume = "0.10"
 futures = "0.3"
 hex = "0.4"

src/agent/onefuzz-task/src/local/cmd.rs

@@ -9,7 +9,7 @@ use crate::local::{
     libfuzzer_test_input, radamsa, test_input, tui::TerminalUi,
 };
 use anyhow::{Context, Result};
-use clap::{App, Arg, SubCommand};
+use clap::{Arg, ArgAction, Command};
 use std::str::FromStr;
 use std::time::Duration;
 use strum::IntoEnumIterator;
@@ -37,17 +37,22 @@ enum Commands {
 const TIMEOUT: &str = "timeout";
 const TUI: &str = "tui";
 
-pub async fn run(args: clap::ArgMatches<'static>) -> Result<()> {
-    let running_duration = value_t!(args, TIMEOUT, u64).ok();
-    let start_ui = args.is_present(TUI);
+pub async fn run(args: clap::ArgMatches) -> Result<()> {
+    let running_duration = args.get_one::<u64>(TIMEOUT).copied();
 
-    let (cmd, sub_args) = args.subcommand();
-    let command =
-        Commands::from_str(cmd).with_context(|| format!("unexpected subcommand: {cmd}"))?;
+    let start_ui = args.get_flag(TUI);
 
-    let sub_args = sub_args
-        .ok_or_else(|| anyhow!("missing subcommand arguments"))?
-        .to_owned();
+    let (cmd, sub_args) = args.subcommand().ok_or_else(|| {
+        format_err!(
+            "Expected subcommand for 'local'. Use 'local help' to see available subcommands."
+        )
+    })?;
+
+    let command = Commands::from_str(cmd).with_context(|| {
+        format!("Unexpected subcommand: {cmd}. Use 'local help' to see available subcommands.")
+    })?;
+
+    let sub_args = sub_args.clone();
 
     let terminal = if start_ui {
         Some(TerminalUi::init()?)
@@ -104,20 +109,20 @@ pub async fn run(args: clap::ArgMatches<'static>) -> Result<()> {
     }
 }
 
-pub fn args(name: &str) -> App<'static, 'static> {
-    let mut cmd = SubCommand::with_name(name)
+pub fn args(name: &'static str) -> Command {
+    let mut cmd = Command::new(name)
         .about("pre-release local fuzzing")
         .arg(
-            Arg::with_name(TIMEOUT)
+            Arg::new(TIMEOUT)
                 .long(TIMEOUT)
-                .help("The maximum running time in seconds")
-                .takes_value(true),
+                .value_parser(value_parser!(u64))
+                .help("The maximum running time in seconds"),
         )
         .arg(
-            Arg::with_name(TUI)
+            Arg::new(TUI)
                 .long(TUI)
                 .help("Enable the terminal UI")
-                .takes_value(false),
+                .action(ArgAction::SetTrue),
         );
 
     for subcommand in Commands::iter() {

src/agent/onefuzz-task/src/local/common.rs

@@ -7,7 +7,7 @@ use std::{
 
 use anyhow::Result;
 use backoff::{future::retry, Error as BackoffError, ExponentialBackoff};
-use clap::{App, Arg, ArgMatches};
+use clap::{Arg, ArgAction, ArgMatches, Command};
 use flume::Sender;
 use onefuzz::{
     blob::url::BlobContainerUrl, machine_id::MachineIdentity, monitor::DirectoryMonitor,
@@ -76,40 +76,41 @@ pub struct LocalContext {
     pub event_sender: Option<Sender<UiEvent>>,
 }
 
-pub fn get_hash_map(args: &clap::ArgMatches<'_>, name: &str) -> Result<HashMap<String, String>> {
+pub fn get_hash_map(args: &clap::ArgMatches, name: &str) -> Result<HashMap<String, String>> {
     let mut env = HashMap::new();
-    for opt in args.values_of_lossy(name).unwrap_or_default() {
+    for opt in args.get_many::<String>(name).unwrap_or_default() {
         let (k, v) = parse_key_value(opt)?;
         env.insert(k, v);
     }
     Ok(env)
 }
 
-pub fn get_cmd_exe(cmd_type: CmdType, args: &clap::ArgMatches<'_>) -> Result<String> {
+pub fn get_cmd_exe(cmd_type: CmdType, args: &clap::ArgMatches) -> Result<String> {
     let name = match cmd_type {
         CmdType::Target => TARGET_EXE,
         // CmdType::Supervisor => SUPERVISOR_EXE,
         CmdType::Generator => GENERATOR_EXE,
     };
 
-    let exe = value_t!(args, name, String)?;
-    Ok(exe)
+    args.get_one::<String>(name)
+        .cloned()
+        .ok_or_else(|| format_err!("missing argument {name}"))
 }
 
-pub fn get_cmd_arg(cmd_type: CmdType, args: &clap::ArgMatches<'_>) -> Vec<String> {
+pub fn get_cmd_arg(cmd_type: CmdType, args: &clap::ArgMatches) -> Vec<String> {
     let name = match cmd_type {
         CmdType::Target => TARGET_OPTIONS,
         // CmdType::Supervisor => SUPERVISOR_OPTIONS,
         CmdType::Generator => GENERATOR_OPTIONS,
     };
 
-    args.values_of_lossy(name).unwrap_or_default()
+    args.get_many::<String>(name)
+        .unwrap_or_default()
+        .cloned()
+        .collect()
 }
 
-pub fn get_cmd_env(
-    cmd_type: CmdType,
-    args: &clap::ArgMatches<'_>,
-) -> Result<HashMap<String, String>> {
+pub fn get_cmd_env(cmd_type: CmdType, args: &clap::ArgMatches) -> Result<HashMap<String, String>> {
     let env_name = match cmd_type {
         CmdType::Target => TARGET_ENV,
         // CmdType::Supervisor => SUPERVISOR_ENV,
@@ -118,58 +119,58 @@ pub fn get_cmd_env(
     get_hash_map(args, env_name)
 }
 
-pub fn add_common_config(app: App<'static, 'static>) -> App<'static, 'static> {
+pub fn add_common_config(app: Command) -> Command {
     app.arg(
-        Arg::with_name("job_id")
+        Arg::new("job_id")
             .long("job_id")
-            .takes_value(true)
-            .required(false),
+            .required(false)
+            .value_parser(value_parser!(uuid::Uuid)),
     )
     .arg(
-        Arg::with_name("task_id")
+        Arg::new("task_id")
             .long("task_id")
-            .takes_value(true)
-            .required(false),
+            .required(false)
+            .value_parser(value_parser!(uuid::Uuid)),
     )
     .arg(
-        Arg::with_name("instance_id")
+        Arg::new("instance_id")
             .long("instance_id")
-            .takes_value(true)
-            .required(false),
+            .required(false)
+            .value_parser(value_parser!(uuid::Uuid)),
     )
     .arg(
-        Arg::with_name("setup_dir")
+        Arg::new("setup_dir")
             .long("setup_dir")
-            .takes_value(true)
-            .required(false),
+            .required(false)
+            .value_parser(value_parser!(PathBuf)),
     )
     .arg(
-        Arg::with_name(CREATE_JOB_DIR)
+        Arg::new(CREATE_JOB_DIR)
             .long(CREATE_JOB_DIR)
+            .action(ArgAction::SetTrue)
             .required(false)
             .help("create a local job directory to sync the files"),
     )
 }
 
-fn get_uuid(name: &str, args: &ArgMatches<'_>) -> Result<Uuid> {
-    value_t!(args, name, String).map(|x| {
-        Uuid::parse_str(&x).map_err(|x| format_err!("invalid {}.  uuid expected.  {})", name, x))
-    })?
+fn get_uuid(name: &str, args: &ArgMatches) -> Result<Uuid> {
+    args.get_one::<Uuid>(name)
+        .copied()
+        .ok_or_else(|| format_err!("missing argument {name}"))
 }
 
 pub fn get_synced_dirs(
     name: &str,
     job_id: Uuid,
     task_id: Uuid,
-    args: &ArgMatches<'_>,
+    args: &ArgMatches,
 ) -> Result<Vec<SyncedDir>> {
-    let create_job_dir = args.is_present(CREATE_JOB_DIR);
+    let create_job_dir = args.get_flag(CREATE_JOB_DIR);
     let current_dir = std::env::current_dir()?;
-    args.values_of_os(name)
+    args.get_many::<PathBuf>(name)
         .ok_or_else(|| anyhow!("argument '{}' not specified", name))?
         .enumerate()
-        .map(|(index, remote_path)| {
-            let path = PathBuf::from(remote_path);
+        .map(|(index, path)| {
             if create_job_dir {
                 let remote_path = path.absolutize()?;
                 let remote_url = Url::from_file_path(remote_path).expect("invalid file path");
@@ -182,7 +183,7 @@ pub fn get_synced_dirs(
             } else {
                 Ok(SyncedDir {
                     remote_path: None,
-                    local_path: path,
+                    local_path: path.clone(),
                 })
             }
         })
@@ -193,10 +194,14 @@ pub fn get_synced_dir(
     name: &str,
     job_id: Uuid,
     task_id: Uuid,
-    args: &ArgMatches<'_>,
+    args: &ArgMatches,
 ) -> Result<SyncedDir> {
-    let remote_path = value_t!(args, name, PathBuf)?.absolutize()?.into_owned();
-    if args.is_present(CREATE_JOB_DIR) {
+    let remote_path = args
+        .get_one::<PathBuf>(name)
+        .ok_or_else(|| format_err!("missing argument {name}"))?
+        .absolutize()?
+        .into_owned();
+    if args.get_flag(CREATE_JOB_DIR) {
         let remote_url =
             Url::from_file_path(remote_path).map_err(|_| anyhow!("invalid file path"))?;
         let remote_blob_url = BlobContainerUrl::new(remote_url)?;
@@ -218,24 +223,24 @@ pub fn get_synced_dir(
 // enables making the one-shot crash report generation, which isn't really a task,
 // consistent across multiple runs.
 pub async fn build_local_context(
-    args: &ArgMatches<'_>,
+    args: &ArgMatches,
     generate_task_id: bool,
     event_sender: Option<Sender<UiEvent>>,
 ) -> Result<LocalContext> {
-    let job_id = get_uuid("job_id", args).unwrap_or_else(|_| Uuid::nil());
+    let job_id = get_uuid("job_id", args).unwrap_or_default();
     let task_id = get_uuid("task_id", args).unwrap_or_else(|_| {
         if generate_task_id {
             Uuid::new_v4()
         } else {
             Uuid::nil()
         }
     });
-    let instance_id = get_uuid("instance_id", args).unwrap_or_else(|_| Uuid::nil());
+    let instance_id = get_uuid("instance_id", args).unwrap_or_default();
 
-    let setup_dir = if args.is_present(SETUP_DIR) {
-        value_t!(args, SETUP_DIR, PathBuf)?
-    } else if args.is_present(TARGET_EXE) {
-        value_t!(args, TARGET_EXE, PathBuf)?
+    let setup_dir = if let Some(setup_dir) = args.get_one::<PathBuf>(SETUP_DIR) {
+        setup_dir.clone()
+    } else if let Some(target_exe) = args.get_one::<String>(TARGET_EXE) {
+        PathBuf::from(target_exe)
             .parent()
             .map(|x| x.to_path_buf())
             .unwrap_or_default()