From aa83f20ad8a92beaba2a88379d5791a40914b812 Mon Sep 17 00:00:00 2001 From: Jason Shirk Date: Wed, 10 Mar 2021 23:36:03 -0800 Subject: [PATCH] Simplify types used in stack walking (#657) * Fix test build * Unify stack frame types * Always include code offset within a function * Run cargo test --workspace in CI --- src/agent/coverage/src/code/tests.rs | 29 ++-- src/agent/debugger/src/stack.rs | 126 +++++++++++------- src/agent/input-tester/src/crash_detector.rs | 1 + src/agent/input-tester/src/test_result/mod.rs | 97 +------------- src/agent/reqwest-retry/src/lib.rs | 7 +- src/ci/agent.sh | 5 +- 6 files changed, 110 insertions(+), 155 deletions(-) diff --git a/src/agent/coverage/src/code/tests.rs b/src/agent/coverage/src/code/tests.rs index 6783ec8fd..390fcc68a 100644 --- a/src/agent/coverage/src/code/tests.rs +++ b/src/agent/coverage/src/code/tests.rs @@ -103,6 +103,18 @@ macro_rules! from_json { }}; } +#[cfg(target_os = "windows")] +const EXE: &str = r"c:\bin\fuzz.exe"; + +#[cfg(target_os = "linux")] +const EXE: &str = "/bin/fuzz.exe"; + +#[cfg(target_os = "windows")] +const LIB: &str = r"c:\lib\libpthread.dll"; + +#[cfg(target_os = "linux")] +const LIB: &str = "/lib/libpthread.so.0"; + fn module(s: &str) -> ModulePath { ModulePath::new(s.into()).unwrap() } @@ -112,7 +124,8 @@ fn test_cmd_filter_empty_def() { let filter = from_json!([]); // All modules and symbols are included by default. - let exe = module("/bin/fuzz.exe"); + + let exe = module(EXE); assert!(filter.includes_module(&exe)); assert!(filter.includes_symbol(&exe, "main")); assert!(filter.includes_symbol(&exe, "_start")); @@ -120,7 +133,7 @@ fn test_cmd_filter_empty_def() { assert!(filter.includes_symbol(&exe, "__asan_memcpy")); assert!(filter.includes_symbol(&exe, "__asan_load8")); - let lib = module("/lib/libpthread.so.0"); + let lib = module(LIB); assert!(filter.includes_module(&lib)); assert!(filter.includes_symbol(&lib, "pthread_join")); assert!(filter.includes_symbol(&lib, "pthread_yield")); @@ -136,7 +149,7 @@ fn test_cmd_filter_module_include_list() { ]); // The filtered module and its matching symbols are included. - let exe = module("/bin/fuzz.exe"); + let exe = module(EXE); assert!(filter.includes_module(&exe)); assert!(!filter.includes_symbol(&exe, "_start")); assert!(filter.includes_symbol(&exe, "main")); @@ -145,7 +158,7 @@ fn test_cmd_filter_module_include_list() { assert!(!filter.includes_symbol(&exe, "__asan_load8")); // Other modules and their symbols are included by default. - let lib = module("/lib/libpthread.so.0"); + let lib = module(LIB); assert!(filter.includes_module(&lib)); assert!(filter.includes_symbol(&lib, "pthread_join")); assert!(filter.includes_symbol(&lib, "pthread_yield")); @@ -163,7 +176,7 @@ fn test_cmd_filter_exclude_list() { ]); // The filtered module is included, and its matching symbols are excluded. - let exe = module("/bin/fuzz.exe"); + let exe = module(EXE); assert!(filter.includes_module(&exe)); assert!(!filter.includes_symbol(&exe, "_start")); assert!(filter.includes_symbol(&exe, "main")); @@ -173,7 +186,7 @@ fn test_cmd_filter_exclude_list() { assert!(!filter.includes_symbol(&exe, "_start")); // Other modules and their symbols are included by default. - let lib = module("/lib/libpthread.so.0"); + let lib = module(LIB); assert!(filter.includes_module(&lib)); assert!(filter.includes_symbol(&lib, "pthread_join")); assert!(filter.includes_symbol(&lib, "pthread_yield")); @@ -197,7 +210,7 @@ fn test_cmd_filter_include_list_and_exclude_default() { ]); // The filtered module is included, and only matching rules are included. - let exe = module("/bin/fuzz.exe"); + let exe = module(EXE); assert!(filter.includes_module(&exe)); assert!(!filter.includes_symbol(&exe, "_start")); assert!(filter.includes_symbol(&exe, "main")); @@ -206,7 +219,7 @@ fn test_cmd_filter_include_list_and_exclude_default() { assert!(!filter.includes_symbol(&exe, "__asan_load8")); // Other modules and their symbols are excluded by default. - let lib = module("/lib/libpthread.so.0"); + let lib = module(LIB); assert!(!filter.includes_module(&lib)); assert!(!filter.includes_symbol(&lib, "pthread_yield")); assert!(!filter.includes_symbol(&lib, "pthread_join")); diff --git a/src/agent/debugger/src/stack.rs b/src/agent/debugger/src/stack.rs index 17e083912..26565c7ac 100644 --- a/src/agent/debugger/src/stack.rs +++ b/src/agent/debugger/src/stack.rs @@ -14,12 +14,33 @@ use serde::{Serialize, Serializer}; use win_util::memory; use winapi::{shared::minwindef::DWORD, um::winnt::HANDLE}; -use crate::dbghelp::{self, DebugHelpGuard, ModuleInfo}; +use crate::dbghelp::{self, DebugHelpGuard, ModuleInfo, SymLineInfo}; const UNKNOWN_MODULE: &str = ""; +/// The file and line number for frames in the call stack. #[derive(Clone, Debug, Hash, PartialEq)] -pub enum DebugFunctionLocation { +pub struct FileInfo { + pub file: String, + pub line: u32, +} + +impl Display for FileInfo { + fn fmt(&self, formatter: &mut Formatter<'_>) -> fmt::Result { + write!(formatter, "{}:{}", self.file, self.line) + } +} + +impl From<&SymLineInfo> for FileInfo { + fn from(sym_line_info: &SymLineInfo) -> Self { + let file = sym_line_info.filename().to_string_lossy().into(); + let line = sym_line_info.line_number(); + FileInfo { file, line } + } +} + +#[derive(Clone, Debug, Hash, PartialEq)] +pub struct DebugFunctionLocation { /// File/line if available /// /// Should be stable - ASLR and JIT should not change source position, @@ -27,18 +48,35 @@ pub enum DebugFunctionLocation { /// /// We mitigate this loss of precision by collecting multiple samples /// for the same hash bucket. - Line { file: String, line: u32 }, + pub file_info: Option, /// Offset if line information not available. - Offset { disp: u64 }, + pub displacement: u64, +} + +impl DebugFunctionLocation { + pub fn new(displacement: u64) -> Self { + DebugFunctionLocation { + displacement, + file_info: None, + } + } + + pub fn new_with_file_info(displacement: u64, file_info: FileInfo) -> Self { + DebugFunctionLocation { + displacement, + file_info: Some(file_info), + } + } } impl Display for DebugFunctionLocation { fn fmt(&self, formatter: &mut Formatter) -> fmt::Result { - match self { - DebugFunctionLocation::Line { file, line } => write!(formatter, "{}:{}", file, line)?, - DebugFunctionLocation::Offset { disp } => write!(formatter, "0x{:x}", disp)?, - }; + if let Some(file_info) = &self.file_info { + write!(formatter, "{}", file_info)?; + } else { + write!(formatter, "0x{:x}", self.displacement)?; + } Ok(()) } } @@ -72,14 +110,13 @@ impl DebugStackFrame { impl Display for DebugStackFrame { fn fmt(&self, formatter: &mut Formatter) -> fmt::Result { match self { - DebugStackFrame::Frame { function, location } => match location { - DebugFunctionLocation::Line { file, line } => { - write!(formatter, "{} {}:{}", function, file, line) + DebugStackFrame::Frame { function, location } => { + if let Some(file_info) = &location.file_info { + write!(formatter, "{} {}", function, file_info) + } else { + write!(formatter, "{}+0x{:x}", function, location.displacement) } - DebugFunctionLocation::Offset { disp } => { - write!(formatter, "{}+0x{:x}", function, disp) - } - }, + } DebugStackFrame::CorruptFrame => formatter.write_str(""), } } @@ -96,7 +133,7 @@ impl Serialize for DebugStackFrame { #[derive(Debug, PartialEq, Serialize)] pub struct DebugStack { - frames: Vec, + pub frames: Vec, } impl DebugStack { @@ -104,10 +141,6 @@ impl DebugStack { DebugStack { frames } } - pub fn frames(&self) -> &[DebugStackFrame] { - &self.frames - } - pub fn stable_hash(&self) -> u64 { // Corrupted stacks and jit can result in stacks that vary from run to run, so we exclude // those frames and anything below them for a more stable hash. @@ -131,7 +164,7 @@ impl DebugStack { impl Display for DebugStack { fn fmt(&self, formatter: &mut Formatter) -> fmt::Result { let mut first = true; - for frame in self.frames() { + for frame in &self.frames { if !first { writeln!(formatter)?; } @@ -161,27 +194,21 @@ fn get_function_location_in_module( let sym_line_info = dbghlp.sym_get_file_and_line(process_handle, program_counter, inline_context); + let displacement = sym_info.displacement(); let location = match sym_line_info { // Don't use file/line for these magic line numbers. Ok(ref sym_line_info) if !sym_line_info.is_fake_line_number() => { - DebugFunctionLocation::Line { - file: sym_line_info.filename().to_string_lossy().into(), - line: sym_line_info.line_number(), - } + DebugFunctionLocation::new_with_file_info(displacement, sym_line_info.into()) } - _ => DebugFunctionLocation::Offset { - disp: sym_info.displacement(), - }, + _ => DebugFunctionLocation::new(displacement), }; DebugStackFrame::new(function, location) } else { // No function - assume we have an exe with no pdb (so no exports). This should be // common, so we won't report an error. We do want a nice(ish) location though. - let location = DebugFunctionLocation::Offset { - disp: program_counter - module_info.base_address(), - }; + let location = DebugFunctionLocation::new(program_counter - module_info.base_address()); DebugStackFrame::new(module_info.name().to_string_lossy().into(), location) } } @@ -199,7 +226,7 @@ fn get_frame_with_unknown_module(process_handle: HANDLE, program_counter: u64) - .checked_sub(mi.base_address()) .expect("logic error computing fake rva"); - let location = DebugFunctionLocation::Offset { disp: offset }; + let location = DebugFunctionLocation::new(offset); DebugStackFrame::new(UNKNOWN_MODULE.into(), location) } else { DebugStackFrame::corrupt_frame() @@ -266,19 +293,19 @@ mod test { macro_rules! frame { ($name: expr, disp: $disp: expr) => { - DebugStackFrame::new( - $name.to_string(), - DebugFunctionLocation::Offset { disp: $disp }, - ) + DebugStackFrame::new($name.to_string(), DebugFunctionLocation::new($disp)) }; - ($name: expr, line: ($file: expr, $line: expr)) => { + ($name: expr, disp: $disp: expr, line: ($file: expr, $line: expr)) => { DebugStackFrame::new( $name.to_string(), - DebugFunctionLocation::Line { - file: $file.to_string(), - line: $line, - }, + DebugFunctionLocation::new_with_file_info( + $disp, + FileInfo { + file: $file.to_string(), + line: $line, + }, + ), ) }; } @@ -287,21 +314,22 @@ mod test { fn stable_stack_hash() { let frames = vec![ frame!("ntdll", disp: 88442200), - frame!("usage", line: ("foo.c", 88)), - frame!("main", line: ("foo.c", 42)), + frame!("usage", disp: 10, line: ("foo.c", 88)), + frame!("main", disp: 20, line: ("foo.c", 42)), ]; let stack = DebugStack::new(frames); - // Hard coded hash constant is what we want to ensure the hash function is stable. - assert_eq!(stack.stable_hash(), 8083364444338290471); + // Hard coded hash constant is what we want to ensure + // the hash function is relatively stable. + assert_eq!(stack.stable_hash(), 4643290346391834992); } #[test] fn stable_hash_ignore_jit() { let mut frames = vec![ frame!("ntdll", disp: 88442200), - frame!("usage", line: ("foo.c", 88)), - frame!("main", line: ("foo.c", 42)), + frame!("usage", disp: 10, line: ("foo.c", 88)), + frame!("main", disp: 20, line: ("foo.c", 42)), ]; let base_frames = frames.clone(); @@ -318,8 +346,8 @@ mod test { fn stable_hash_assuming_stack_corrupted() { let mut frames = vec![ frame!("ntdll", disp: 88442200), - frame!("usage", line: ("foo.c", 88)), - frame!("main", line: ("foo.c", 42)), + frame!("usage", disp: 10, line: ("foo.c", 88)), + frame!("main", disp: 20, line: ("foo.c", 42)), ]; let base_frames = frames.clone(); diff --git a/src/agent/input-tester/src/crash_detector.rs b/src/agent/input-tester/src/crash_detector.rs index 7a3ffe540..977a77160 100644 --- a/src/agent/input-tester/src/crash_detector.rs +++ b/src/agent/input-tester/src/crash_detector.rs @@ -423,6 +423,7 @@ mod tests { test_process( r"C:\windows\system32\WindowsPowerShell\v1.0\powershell.exe", &vec!["/nop".to_string(), "/c".to_string(), $script.to_string()], + &HashMap::default(), $timeout, /*ignore first chance exceptions*/ true, None, diff --git a/src/agent/input-tester/src/test_result/mod.rs b/src/agent/input-tester/src/test_result/mod.rs index ef5d81cac..a3a08802d 100644 --- a/src/agent/input-tester/src/test_result/mod.rs +++ b/src/agent/input-tester/src/test_result/mod.rs @@ -10,7 +10,7 @@ pub mod verifier_stop; use std::{fmt, path::Path}; -use debugger::stack; +use debugger::stack::{DebugStack, DebugStackFrame}; use log::error; use win_util::process; use winapi::um::{ @@ -169,14 +169,15 @@ pub fn new_exception_description( pub fn new_exception( process_handle: HANDLE, exception: &EXCEPTION_DEBUG_INFO, - stack: stack::DebugStack, + stack: DebugStack, ) -> Exception { + let stack_hash = stack.stable_hash(); Exception { exception_code: exception.ExceptionRecord.ExceptionCode, description: new_exception_description(process_handle, &exception.ExceptionRecord), - stack_hash: stack.stable_hash(), + stack_hash, first_chance: exception.dwFirstChance != 0, - stack_frames: stack.frames().iter().map(|f| f.into()).collect(), + stack_frames: stack.frames, } } @@ -196,94 +197,6 @@ pub fn new_test_result( } } -/// The file and line number for frame in the calls stack. -#[derive(Clone)] -pub struct FileInfo { - pub file: String, - pub line: u32, -} - -/// The location within a function for a call stack entry. -#[derive(Clone)] -pub enum DebugFunctionLocation { - /// If symbol information is available, we use the file/line numbers for stability across builds. - FileInfo(FileInfo), - /// If no symbol information is available, the offset within the function is used. - Displacement(u64), -} - -impl fmt::Display for DebugFunctionLocation { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - match self { - DebugFunctionLocation::FileInfo(file_info) => { - write!(formatter, "{}:{}", file_info.file, file_info.line) - } - DebugFunctionLocation::Displacement(disp) => write!(formatter, "0x{:x}", disp), - } - } -} - -impl<'a> From<&'a stack::DebugFunctionLocation> for DebugFunctionLocation { - fn from(location: &'a stack::DebugFunctionLocation) -> Self { - match location { - stack::DebugFunctionLocation::Line { file, line } => { - DebugFunctionLocation::FileInfo(FileInfo { - file: file.to_string(), - line: *line, - }) - } - - stack::DebugFunctionLocation::Offset { disp } => { - DebugFunctionLocation::Displacement(*disp) - } - } - } -} - -/// A stack frame for reporting where an exception or other bug occurs. -#[derive(Clone)] -pub enum DebugStackFrame { - Frame { - /// The name of the function (if available via symbols or exports) or possibly something else like a - /// (possibly synthetic) module name. - function: String, - - /// Location details such as file/line (if symbols are available) or offset - location: DebugFunctionLocation, - }, - - CorruptFrame, -} - -impl fmt::Display for DebugStackFrame { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - match self { - DebugStackFrame::Frame { function, location } => { - formatter.write_str(function)?; - match location { - DebugFunctionLocation::FileInfo(file_info) => { - write!(formatter, " {}:{}", file_info.file, file_info.line) - } - DebugFunctionLocation::Displacement(disp) => write!(formatter, "+0x{:x}", disp), - } - } - DebugStackFrame::CorruptFrame => formatter.write_str(""), - } - } -} - -impl<'a> From<&'a stack::DebugStackFrame> for DebugStackFrame { - fn from(frame: &'a stack::DebugStackFrame) -> Self { - match frame { - stack::DebugStackFrame::Frame { function, location } => DebugStackFrame::Frame { - function: function.to_string(), - location: location.into(), - }, - stack::DebugStackFrame::CorruptFrame => DebugStackFrame::CorruptFrame, - } - } -} - /// The details of an exception observed by the execution engine. #[derive(Clone)] pub struct Exception { diff --git a/src/agent/reqwest-retry/src/lib.rs b/src/agent/reqwest-retry/src/lib.rs index 019f32d75..f66b4cdce 100644 --- a/src/agent/reqwest-retry/src/lib.rs +++ b/src/agent/reqwest-retry/src/lib.rs @@ -205,9 +205,12 @@ mod test { if let Err(err) = &resp { let as_text = format!("{}", err); - assert!(as_text.contains("Maximum number of attempts reached for this request")); + assert!( + as_text.contains("Maximum number of attempts reached for this request") + || as_text.contains("os error 10061") + ); } else { - bail!("response to {} was expected to fail", invalid_url); + anyhow::bail!("response to {} was expected to fail", invalid_url); } Ok(()) diff --git a/src/ci/agent.sh b/src/ci/agent.sh index 001295977..e7ab6c3cb 100755 --- a/src/ci/agent.sh +++ b/src/ci/agent.sh @@ -38,13 +38,10 @@ 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 +cargo test --release --workspace # TODO: re-enable integration tests. # cargo test --release --manifest-path ./onefuzz-agent/Cargo.toml --features integration_test -- --nocapture -cargo test --release --manifest-path ./onefuzz-agent/Cargo.toml - -cargo test --release --manifest-path ./onefuzz/Cargo.toml cp target/release/onefuzz-agent* ../../artifacts/agent cp target/release/onefuzz-supervisor* ../../artifacts/agent