From f2c9b72b0ccfe763fa54b4f20b921d1749c37083 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 22:31:05 -0500 Subject: [PATCH 01/10] cargo clippy fixes --- src/agent/onefuzz-agent/src/local/common.rs | 30 ++++++------- .../onefuzz-agent/src/tasks/fuzz/generator.rs | 15 ++++--- .../onefuzz-agent/src/tasks/report/generic.rs | 12 +++--- src/agent/onefuzz/src/input_tester.rs | 43 +++++++++++++------ src/agent/onefuzz/src/libfuzzer.rs | 16 +++---- 5 files changed, 65 insertions(+), 51 deletions(-) diff --git a/src/agent/onefuzz-agent/src/local/common.rs b/src/agent/onefuzz-agent/src/local/common.rs index 2fcc292db1..f1343b3c23 100644 --- a/src/agent/onefuzz-agent/src/local/common.rs +++ b/src/agent/onefuzz-agent/src/local/common.rs @@ -26,9 +26,9 @@ pub const CHECK_FUZZER_HELP: &str = "check_fuzzer_help"; pub const TARGET_EXE: &str = "target_exe"; pub const TARGET_ENV: &str = "target_env"; pub const TARGET_OPTIONS: &str = "target_options"; -pub const SUPERVISOR_EXE: &str = "supervisor_exe"; -pub const SUPERVISOR_ENV: &str = "supervisor_env"; -pub const SUPERVISOR_OPTIONS: &str = "supervisor_options"; +// pub const SUPERVISOR_EXE: &str = "supervisor_exe"; +// pub const SUPERVISOR_ENV: &str = "supervisor_env"; +// pub const SUPERVISOR_OPTIONS: &str = "supervisor_options"; pub const GENERATOR_EXE: &str = "generator_exe"; pub const GENERATOR_ENV: &str = "generator_env"; pub const GENERATOR_OPTIONS: &str = "generator_options"; @@ -36,7 +36,7 @@ pub const GENERATOR_OPTIONS: &str = "generator_options"; pub enum CmdType { Target, Generator, - Supervisor, + // Supervisor, } pub fn add_cmd_options( @@ -48,7 +48,7 @@ pub fn add_cmd_options( ) -> App<'static, 'static> { let (exe_name, env_name, arg_name) = match cmd_type { CmdType::Target => (TARGET_EXE, TARGET_ENV, TARGET_OPTIONS), - CmdType::Supervisor => (SUPERVISOR_EXE, SUPERVISOR_ENV, SUPERVISOR_OPTIONS), + // CmdType::Supervisor => (SUPERVISOR_EXE, SUPERVISOR_ENV, SUPERVISOR_OPTIONS), CmdType::Generator => (GENERATOR_EXE, GENERATOR_ENV, GENERATOR_OPTIONS), }; @@ -78,7 +78,7 @@ pub fn add_cmd_options( pub fn get_cmd_exe(cmd_type: CmdType, args: &clap::ArgMatches<'_>) -> Result { let name = match cmd_type { CmdType::Target => TARGET_EXE, - CmdType::Supervisor => SUPERVISOR_EXE, + // CmdType::Supervisor => SUPERVISOR_EXE, CmdType::Generator => GENERATOR_EXE, }; @@ -89,7 +89,7 @@ pub fn get_cmd_exe(cmd_type: CmdType, args: &clap::ArgMatches<'_>) -> Result) -> Vec { let name = match cmd_type { CmdType::Target => TARGET_OPTIONS, - CmdType::Supervisor => SUPERVISOR_OPTIONS, + // CmdType::Supervisor => SUPERVISOR_OPTIONS, CmdType::Generator => GENERATOR_OPTIONS, }; @@ -102,7 +102,7 @@ pub fn get_cmd_env( ) -> Result> { let env_name = match cmd_type { CmdType::Target => TARGET_ENV, - CmdType::Supervisor => SUPERVISOR_ENV, + // CmdType::Supervisor => SUPERVISOR_ENV, CmdType::Generator => GENERATOR_ENV, }; @@ -156,15 +156,13 @@ pub fn build_common_config(args: &ArgMatches<'_>) -> Result { 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)? + .parent() + .map(|x| x.to_path_buf()) + .unwrap_or_default() } else { - if args.is_present(TARGET_EXE) { - value_t!(args, TARGET_EXE, PathBuf)? - .parent() - .map(|x| x.to_path_buf()) - .unwrap_or_default() - } else { - PathBuf::default() - } + PathBuf::default() }; let config = CommonConfig { diff --git a/src/agent/onefuzz-agent/src/tasks/fuzz/generator.rs b/src/agent/onefuzz-agent/src/tasks/fuzz/generator.rs index 72c4c63668..be7de2d914 100644 --- a/src/agent/onefuzz-agent/src/tasks/fuzz/generator.rs +++ b/src/agent/onefuzz-agent/src/tasks/fuzz/generator.rs @@ -91,18 +91,21 @@ impl GeneratorTask { } async fn fuzzing_loop(&self, heartbeat_client: Option) -> Result<()> { - let tester = Tester::new( + let mut tester = Tester::new( &self.config.common.setup_dir, &self.config.target_exe, &self.config.target_options, &self.config.target_env, - &self.config.target_timeout, - self.config.check_asan_log, - false, - self.config.check_debugger, - self.config.check_retry_count, ); + tester + .check_asan_log(self.config.check_asan_log) + .check_debugger(self.config.check_debugger) + .check_retry_count(self.config.check_retry_count); + if let Some(timeout) = self.config.target_timeout { + tester.timeout(timeout); + } + loop { for corpus_dir in &self.config.readonly_inputs { heartbeat_client.alive(); diff --git a/src/agent/onefuzz-agent/src/tasks/report/generic.rs b/src/agent/onefuzz-agent/src/tasks/report/generic.rs index db1893ac63..b6857d082e 100644 --- a/src/agent/onefuzz-agent/src/tasks/report/generic.rs +++ b/src/agent/onefuzz-agent/src/tasks/report/generic.rs @@ -118,18 +118,18 @@ pub struct GenericReportProcessor<'a> { impl<'a> GenericReportProcessor<'a> { pub fn new(config: &'a Config, heartbeat_client: Option) -> Self { - let tester = Tester::new( + let mut tester = Tester::new( &config.common.setup_dir, &config.target_exe, &config.target_options, &config.target_env, - &config.target_timeout, - config.check_asan_log, - false, - config.check_debugger, - config.check_retry_count, ); + tester + .check_asan_log(config.check_asan_log) + .check_debugger(config.check_debugger) + .check_retry_count(config.check_retry_count); + Self { config, tester, diff --git a/src/agent/onefuzz/src/input_tester.rs b/src/agent/onefuzz/src/input_tester.rs index 9d89cd0e0b..d120c19b98 100644 --- a/src/agent/onefuzz/src/input_tester.rs +++ b/src/agent/onefuzz/src/input_tester.rs @@ -12,7 +12,7 @@ use anyhow::{Error, Result}; use std::{collections::HashMap, path::Path, time::Duration}; use tempfile::tempdir; -const DEFAULT_TIMEOUT_SECS: u64 = 5; +const DEFAULT_TIMEOUT_SECS: Duration = Duration::from_secs(5); const CRASH_SITE_UNAVAILABLE: &str = ""; pub struct Tester<'a> { @@ -47,26 +47,45 @@ impl<'a> Tester<'a> { exe_path: &'a Path, arguments: &'a [String], environ: &'a HashMap, - timeout: &'a Option, - check_asan_log: bool, - check_asan_stderr: bool, - check_debugger: bool, - check_retry_count: u64, ) -> Self { - let timeout = Duration::from_secs(timeout.unwrap_or(DEFAULT_TIMEOUT_SECS)); Self { setup_dir, exe_path, arguments, environ, - timeout, - check_asan_log, - check_asan_stderr, - check_debugger, - check_retry_count, + timeout: DEFAULT_TIMEOUT_SECS, + check_asan_log: false, + check_asan_stderr: false, + check_debugger: true, + check_retry_count: 0, } } + pub fn timeout(&mut self, value: u64) -> &mut Self { + self.timeout = Duration::from_secs(value); + self + } + + pub fn check_asan_log(&mut self, value: bool) -> &mut Self { + self.check_asan_log = value; + self + } + + pub fn check_asan_stderr(&mut self, value: bool) -> &mut Self { + self.check_asan_stderr = value; + self + } + + pub fn check_debugger(&mut self, value: bool) -> &mut Self { + self.check_debugger = value; + self + } + + pub fn check_retry_count(&mut self, value: u64) -> &mut Self { + self.check_retry_count = value; + self + } + #[cfg(target_os = "windows")] async fn test_input_debugger( &self, diff --git a/src/agent/onefuzz/src/libfuzzer.rs b/src/agent/onefuzz/src/libfuzzer.rs index 16552dc4cd..289a522253 100644 --- a/src/agent/onefuzz/src/libfuzzer.rs +++ b/src/agent/onefuzz/src/libfuzzer.rs @@ -162,17 +162,11 @@ impl<'a> LibFuzzer<'a> { let mut options = self.options.to_owned(); options.push("{input}".to_string()); - let tester = Tester::new( - &self.setup_dir, - &self.exe, - &options, - &self.env, - &timeout, - false, - true, - false, - retry, - ); + let mut tester = Tester::new(&self.setup_dir, &self.exe, &options, &self.env); + tester.check_asan_stderr(true).check_retry_count(retry); + if let Some(timeout) = timeout { + tester.timeout(timeout); + } tester.test_input(test_input.as_ref()).await } From aa518cb8a4bd908cc8aec87e39e0f50893b6fd36 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 22:32:12 -0500 Subject: [PATCH 02/10] enforce clippy on agent --- src/ci/agent.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ci/agent.sh b/src/ci/agent.sh index 687164b4be..ee27d786c0 100755 --- a/src/ci/agent.sh +++ b/src/ci/agent.sh @@ -35,6 +35,7 @@ cargo fmt -- --check cargo audit --deny warnings --deny unmaintained --deny unsound --deny yanked --ignore RUSTSEC-2019-0031 --ignore RUSTSEC-2020-0016 --ignore RUSTSEC-2020-0036 cargo-license -j > data/licenses.json cargo build --release --locked +cargo clippy --release -- -D warnings # export RUST_LOG=trace export RUST_BACKTRACE=full cargo test --release --manifest-path ./onefuzz-supervisor/Cargo.toml From e794bd1aa768e772051972210f50d55ea89e72e2 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 22:35:18 -0500 Subject: [PATCH 03/10] spin nested dependency is no longer an issue --- src/ci/agent.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ci/agent.sh b/src/ci/agent.sh index ee27d786c0..5abcb85e44 100755 --- a/src/ci/agent.sh +++ b/src/ci/agent.sh @@ -29,10 +29,9 @@ mkdir -p artifacts/agent cd src/agent cargo fmt -- --check -# RUSTSEC-2019-0031: a dependency spin (pulled in from ring) is not actively maintained # RUSTSEC-2020-0016: a dependency net2 (pulled in from tokio) is deprecated # RUSTSEC-2020-0036: a dependency failure (pulled from proc-maps) is deprecated -cargo audit --deny warnings --deny unmaintained --deny unsound --deny yanked --ignore RUSTSEC-2019-0031 --ignore RUSTSEC-2020-0016 --ignore RUSTSEC-2020-0036 +cargo audit --deny warnings --deny unmaintained --deny unsound --deny yanked --ignore RUSTSEC-2020-0016 --ignore RUSTSEC-2020-0036 cargo-license -j > data/licenses.json cargo build --release --locked cargo clippy --release -- -D warnings From 546831c1833cea6ec5beb06037673c730eed745d Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 23:16:06 -0500 Subject: [PATCH 04/10] cargo-fmt on windows --- src/agent/debugger/src/dbghelp.rs | 2 +- src/agent/debugger/src/target.rs | 33 +++++++++---------- src/agent/input-tester/src/test_result/mod.rs | 10 ++---- src/agent/input-tester/src/tester.rs | 8 ++--- src/agent/onefuzz/src/machine_id.rs | 5 ++- 5 files changed, 27 insertions(+), 31 deletions(-) diff --git a/src/agent/debugger/src/dbghelp.rs b/src/agent/debugger/src/dbghelp.rs index fc5b9ded6e..52d4b0a5c0 100644 --- a/src/agent/debugger/src/dbghelp.rs +++ b/src/agent/debugger/src/dbghelp.rs @@ -602,7 +602,7 @@ impl DebugHelpGuard { ) -> Result { let mut line_info: IMAGEHLP_LINEW64 = unsafe { MaybeUninit::zeroed().assume_init() }; line_info.SizeOfStruct = size_of::() as DWORD; - let mut displacement = 0 as DWORD; + let mut displacement: DWORD = 0; check_winapi(|| unsafe { SymGetLineFromInlineContextW( process_handle, diff --git a/src/agent/debugger/src/target.rs b/src/agent/debugger/src/target.rs index 23a57ec508..a96e96a1cb 100644 --- a/src/agent/debugger/src/target.rs +++ b/src/agent/debugger/src/target.rs @@ -405,14 +405,16 @@ impl Target { bp.set_original_byte(Some(original_byte)); bp.set_id(id); }) - .or_insert(Breakpoint::new( - address, - kind, - /*enabled*/ true, - /*original_byte*/ Some(original_byte), - /*hit_count*/ 0, - id, - )); + .or_insert_with(|| { + Breakpoint::new( + address, + kind, + /*enabled*/ true, + /*original_byte*/ Some(original_byte), + /*hit_count*/ 0, + id, + ) + }); write_instruction_byte(self.process_handle, address, 0xcc)?; @@ -616,18 +618,15 @@ impl Target { pub(crate) fn handle_single_step(&mut self, step_state: StepState) -> Result<()> { self.single_step.remove(&self.current_thread_handle); - match step_state { - StepState::Breakpoint { pc } => { - write_instruction_byte(self.process_handle, pc, 0xcc)?; + if let StepState::Breakpoint { pc } = step_state { + write_instruction_byte(self.process_handle, pc, 0xcc)?; - // Resume all threads if we aren't waiting for any threads to single step. - if self.single_step.is_empty() { - for thread_info in self.thread_info.values_mut() { - thread_info.resume_thread()?; - } + // Resume all threads if we aren't waiting for any threads to single step. + if self.single_step.is_empty() { + for thread_info in self.thread_info.values_mut() { + thread_info.resume_thread()?; } } - _ => {} } Ok(()) diff --git a/src/agent/input-tester/src/test_result/mod.rs b/src/agent/input-tester/src/test_result/mod.rs index 23b2c0ec49..ef5d81cac5 100644 --- a/src/agent/input-tester/src/test_result/mod.rs +++ b/src/agent/input-tester/src/test_result/mod.rs @@ -343,17 +343,11 @@ impl ExitStatus { } pub fn is_normal_exit(&self) -> bool { - match self { - ExitStatus::Code(_) => true, - _ => false, - } + matches!(self, ExitStatus::Code(_)) } pub fn is_timeout(&self) -> bool { - match self { - ExitStatus::Timeout(_) => true, - _ => false, - } + matches!(self, ExitStatus::Timeout(_)) } } diff --git a/src/agent/input-tester/src/tester.rs b/src/agent/input-tester/src/tester.rs index b6a48fed7a..27dae02498 100644 --- a/src/agent/input-tester/src/tester.rs +++ b/src/agent/input-tester/src/tester.rs @@ -420,11 +420,11 @@ impl Tester { } fn exception_from_throw(exception: &Exception) -> bool { - match exception.description { + matches!( + exception.description, ExceptionDescription::GenericException(ExceptionCode::ClrException) - | ExceptionDescription::GenericException(ExceptionCode::CppException) => true, - _ => false, - } + | ExceptionDescription::GenericException(ExceptionCode::CppException) + ) } /// Using heuristics, choose the most serious exception. Typically this would be the one that diff --git a/src/agent/onefuzz/src/machine_id.rs b/src/agent/onefuzz/src/machine_id.rs index 3e3a4ca1ce..5b6c926c21 100644 --- a/src/agent/onefuzz/src/machine_id.rs +++ b/src/agent/onefuzz/src/machine_id.rs @@ -2,8 +2,11 @@ // Licensed under the MIT License. use crate::fs::{onefuzz_etc, write_file}; -use anyhow::{Context, Result}; +#[cfg(target_os = "linux")] +use anyhow::Context; +use anyhow::Result; use reqwest_retry::SendRetry; +#[cfg(target_os = "linux")] use std::path::Path; use std::time::Duration; use tokio::fs; From eb4e71a13492e722eb5a77254bab7519b4119891 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 23:33:15 -0500 Subject: [PATCH 05/10] address feedback --- src/agent/onefuzz/src/input_tester.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent/onefuzz/src/input_tester.rs b/src/agent/onefuzz/src/input_tester.rs index d120c19b98..858dc51148 100644 --- a/src/agent/onefuzz/src/input_tester.rs +++ b/src/agent/onefuzz/src/input_tester.rs @@ -12,7 +12,7 @@ use anyhow::{Error, Result}; use std::{collections::HashMap, path::Path, time::Duration}; use tempfile::tempdir; -const DEFAULT_TIMEOUT_SECS: Duration = Duration::from_secs(5); +const DEFAULT_TIMEOUT: Duration = Duration::from_secs(5); const CRASH_SITE_UNAVAILABLE: &str = ""; pub struct Tester<'a> { @@ -53,7 +53,7 @@ impl<'a> Tester<'a> { exe_path, arguments, environ, - timeout: DEFAULT_TIMEOUT_SECS, + timeout: DEFAULT_TIMEOUT, check_asan_log: false, check_asan_stderr: false, check_debugger: true, From 5fd40aa60f53550eb6cf5350b6bd0ba70364fc48 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 23:35:10 -0500 Subject: [PATCH 06/10] revert change to target. --- src/agent/debugger/src/target.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/agent/debugger/src/target.rs b/src/agent/debugger/src/target.rs index a96e96a1cb..d72b4c5f96 100644 --- a/src/agent/debugger/src/target.rs +++ b/src/agent/debugger/src/target.rs @@ -618,15 +618,18 @@ impl Target { pub(crate) fn handle_single_step(&mut self, step_state: StepState) -> Result<()> { self.single_step.remove(&self.current_thread_handle); - if let StepState::Breakpoint { pc } = step_state { - write_instruction_byte(self.process_handle, pc, 0xcc)?; - - // Resume all threads if we aren't waiting for any threads to single step. - if self.single_step.is_empty() { - for thread_info in self.thread_info.values_mut() { - thread_info.resume_thread()?; + match step_state { + StepState::Breakpoint { pc } => { + write_instruction_byte(self.process_handle, pc, 0xcc)?; + + // Resume all threads if we aren't waiting for any threads to single step. + if self.single_step.is_empty() { + for thread_info in self.thread_info.values_mut() { + thread_info.resume_thread()?; + } } } + _ => {} } Ok(()) From 253cb4987a66f1331a39c82b8a4746a3a586979b Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Fri, 29 Jan 2021 23:41:11 -0500 Subject: [PATCH 07/10] add allow single_match --- src/agent/debugger/src/target.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/agent/debugger/src/target.rs b/src/agent/debugger/src/target.rs index d72b4c5f96..5fb963de35 100644 --- a/src/agent/debugger/src/target.rs +++ b/src/agent/debugger/src/target.rs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#![allow(clippy::single_match)] + use std::{ collections::hash_map, fs, From 41387936880fc24537eac428bbc80317fe97b756 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Tue, 2 Feb 2021 09:14:49 -0500 Subject: [PATCH 08/10] address feedback --- src/agent/input-tester/src/tester.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/agent/input-tester/src/tester.rs b/src/agent/input-tester/src/tester.rs index 27dae02498..b6a48fed7a 100644 --- a/src/agent/input-tester/src/tester.rs +++ b/src/agent/input-tester/src/tester.rs @@ -420,11 +420,11 @@ impl Tester { } fn exception_from_throw(exception: &Exception) -> bool { - matches!( - exception.description, + match exception.description { ExceptionDescription::GenericException(ExceptionCode::ClrException) - | ExceptionDescription::GenericException(ExceptionCode::CppException) - ) + | ExceptionDescription::GenericException(ExceptionCode::CppException) => true, + _ => false, + } } /// Using heuristics, choose the most serious exception. Typically this would be the one that From acae9b13ac53ab2c95d9ccfdbca37c2a69295be8 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Tue, 2 Feb 2021 09:36:14 -0500 Subject: [PATCH 09/10] add allow for match-like-matches-macro --- src/agent/input-tester/src/tester.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/agent/input-tester/src/tester.rs b/src/agent/input-tester/src/tester.rs index b6a48fed7a..cc462c5bfe 100644 --- a/src/agent/input-tester/src/tester.rs +++ b/src/agent/input-tester/src/tester.rs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#![allow(clippy::match-like-matches-macro)] #![allow(clippy::unknown_clippy_lints)] #![allow(clippy::single_component_path_imports)] #![allow(clippy::option_map_or_none)] From ff4eb50a09dc2d2901f4ae7f7ffd70ac9df301e6 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Tue, 2 Feb 2021 09:58:13 -0500 Subject: [PATCH 10/10] why are the warning & allow identifiers different? --- src/agent/input-tester/src/tester.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/agent/input-tester/src/tester.rs b/src/agent/input-tester/src/tester.rs index cc462c5bfe..82bc6d190a 100644 --- a/src/agent/input-tester/src/tester.rs +++ b/src/agent/input-tester/src/tester.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#![allow(clippy::match-like-matches-macro)] +#![allow(clippy::match_like_matches_macro)] #![allow(clippy::unknown_clippy_lints)] #![allow(clippy::single_component_path_imports)] #![allow(clippy::option_map_or_none)]