From 4955ac470a47cb8b9e50cdaa6b3f1144ce07b660 Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 09:54:35 -0400 Subject: [PATCH 1/9] Make modules and coverage allowlist case insensitive on Windows --- src/agent/coverage/src/allowlist.rs | 10 ++++++++-- src/agent/debugger/src/module.rs | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/agent/coverage/src/allowlist.rs b/src/agent/coverage/src/allowlist.rs index 079f415004..fd1f87b353 100644 --- a/src/agent/coverage/src/allowlist.rs +++ b/src/agent/coverage/src/allowlist.rs @@ -2,7 +2,7 @@ // Licensed under the MIT License. use anyhow::Result; -use regex::{Regex, RegexSet}; +use regex::{Regex, RegexSet, RegexBuilder}; use std::path::Path; #[derive(Clone, Debug)] @@ -144,7 +144,13 @@ fn glob_to_regex(expr: &str) -> Result { // Anchor to line start and end. let expr = format!("^{expr}$"); - Ok(Regex::new(&expr)?) + let mut rb = RegexBuilder::new(&expr); + + if cfg!(windows) { + rb.case_insensitive(true); + } + + Ok(rb.build()?) } #[cfg(test)] diff --git a/src/agent/debugger/src/module.rs b/src/agent/debugger/src/module.rs index acea7ace7f..8a55be4702 100644 --- a/src/agent/debugger/src/module.rs +++ b/src/agent/debugger/src/module.rs @@ -47,6 +47,8 @@ impl Module { "???".into() }); + let path = PathBuf::from(path.as_os_str().to_ascii_lowercase()); + let image_details = get_image_details(&path)?; Ok(Module { From cc551a6cebe5ddc5d166b8139b5603fa99369899 Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 10:17:21 -0400 Subject: [PATCH 2/9] Tests and fmt --- src/agent/coverage/src/allowlist.rs | 2 +- src/agent/coverage/tests/snapshot.rs | 10 +++++++++- .../snapshots/snapshot__windows_snapshot_tests.snap | 2 +- .../tests/windows/{inlinee.cpp => Inlinee.cpp} | 0 src/agent/onefuzz/src/expand.rs | 9 ++++++--- 5 files changed, 17 insertions(+), 6 deletions(-) rename src/agent/coverage/tests/windows/{inlinee.cpp => Inlinee.cpp} (100%) diff --git a/src/agent/coverage/src/allowlist.rs b/src/agent/coverage/src/allowlist.rs index fd1f87b353..25d6e42107 100644 --- a/src/agent/coverage/src/allowlist.rs +++ b/src/agent/coverage/src/allowlist.rs @@ -2,7 +2,7 @@ // Licensed under the MIT License. use anyhow::Result; -use regex::{Regex, RegexSet, RegexBuilder}; +use regex::{Regex, RegexBuilder, RegexSet}; use std::path::Path; #[derive(Clone, Debug)] diff --git a/src/agent/coverage/tests/snapshot.rs b/src/agent/coverage/tests/snapshot.rs index 75d524e2da..f929663925 100644 --- a/src/agent/coverage/tests/snapshot.rs +++ b/src/agent/coverage/tests/snapshot.rs @@ -43,7 +43,8 @@ fn windows_snapshot_tests() { }; // filter to just the input test file: - let source_allowlist = AllowList::parse(&input_path.to_string_lossy()).unwrap(); + let source_allowlist = + AllowList::parse(&input_path.to_string_lossy().to_ascii_lowercase()).unwrap(); let exe_cmd = std::process::Command::new(&exe_name); let recorded = coverage::CoverageRecorder::new(exe_cmd) @@ -52,6 +53,13 @@ fn windows_snapshot_tests() { .record() .unwrap(); + // Windows modules should be case insensitive + recorded + .coverage + .modules + .keys() + .for_each(|k| assert_eq!(k.to_string().to_ascii_lowercase(), k.to_string())); + // generate source-line coverage info let source = coverage::source::binary_to_source_coverage(&recorded.coverage, &source_allowlist) diff --git a/src/agent/coverage/tests/snapshots/snapshot__windows_snapshot_tests.snap b/src/agent/coverage/tests/snapshots/snapshot__windows_snapshot_tests.snap index 016717f8ab..12a38f4ef0 100644 --- a/src/agent/coverage/tests/snapshots/snapshot__windows_snapshot_tests.snap +++ b/src/agent/coverage/tests/snapshots/snapshot__windows_snapshot_tests.snap @@ -1,7 +1,7 @@ --- source: coverage/tests/snapshot.rs expression: result -input_file: coverage/tests/windows/inlinee.cpp +input_file: coverage/tests/windows/Inlinee.cpp --- [ ] #include [ ] diff --git a/src/agent/coverage/tests/windows/inlinee.cpp b/src/agent/coverage/tests/windows/Inlinee.cpp similarity index 100% rename from src/agent/coverage/tests/windows/inlinee.cpp rename to src/agent/coverage/tests/windows/Inlinee.cpp diff --git a/src/agent/onefuzz/src/expand.rs b/src/agent/onefuzz/src/expand.rs index 93587a6b58..7f1813899f 100644 --- a/src/agent/onefuzz/src/expand.rs +++ b/src/agent/onefuzz/src/expand.rs @@ -128,7 +128,8 @@ impl<'a> Expand<'a> { fn input_file_sha256(&self) -> Result> { let Some(val) = self.values.get(PlaceHolder::Input.get_string()) else { - bail!("no value found for {}, unable to evaluate {}", + bail!( + "no value found for {}, unable to evaluate {}", PlaceHolder::Input.get_string(), PlaceHolder::InputFileSha256.get_string(), ) @@ -149,7 +150,8 @@ impl<'a> Expand<'a> { fn extract_file_name_no_ext(&self) -> Result> { let Some(val) = self.values.get(PlaceHolder::Input.get_string()) else { - bail!("no value found for {}, unable to evaluate {}", + bail!( + "no value found for {}, unable to evaluate {}", PlaceHolder::Input.get_string(), PlaceHolder::InputFileNameNoExt.get_string(), ) @@ -173,7 +175,8 @@ impl<'a> Expand<'a> { fn extract_file_name(&self) -> Result> { let Some(val) = self.values.get(PlaceHolder::Input.get_string()) else { - bail!("no value found for {}, unable to evaluate {}", + bail!( + "no value found for {}, unable to evaluate {}", PlaceHolder::Input.get_string(), PlaceHolder::InputFileName.get_string(), ) From ab75983f7926bb70c243b07fdd60657bc830f611 Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 15:14:38 -0400 Subject: [PATCH 3/9] PR comments --- src/agent/coverage/src/allowlist.rs | 16 ++++++++-------- src/agent/coverage/src/allowlist/tests.rs | 18 ++++++++++++++++++ src/agent/coverage/src/source.rs | 11 ++++++++--- src/agent/coverage/tests/snapshot.rs | 16 ++++++++-------- src/agent/debugger/src/module.rs | 4 +--- 5 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/agent/coverage/src/allowlist.rs b/src/agent/coverage/src/allowlist.rs index 25d6e42107..9371578480 100644 --- a/src/agent/coverage/src/allowlist.rs +++ b/src/agent/coverage/src/allowlist.rs @@ -2,7 +2,7 @@ // Licensed under the MIT License. use anyhow::Result; -use regex::{Regex, RegexBuilder, RegexSet}; +use regex::{Regex, RegexSet}; use std::path::Path; #[derive(Clone, Debug)] @@ -142,15 +142,15 @@ fn glob_to_regex(expr: &str) -> Result { let expr = expr.replace(r"\*", ".*"); // Anchor to line start and end. - let expr = format!("^{expr}$"); - - let mut rb = RegexBuilder::new(&expr); - - if cfg!(windows) { - rb.case_insensitive(true); + // On Windows we should also ignore case. + let expr = if cfg!(windows) { + format!("(?i)^{expr}$") } + else { + format!("^{expr}$") + }; - Ok(rb.build()?) + Ok(Regex::new(&expr)?) } #[cfg(test)] diff --git a/src/agent/coverage/src/allowlist/tests.rs b/src/agent/coverage/src/allowlist/tests.rs index 0f46ef3df8..8de5448d5a 100644 --- a/src/agent/coverage/src/allowlist/tests.rs +++ b/src/agent/coverage/src/allowlist/tests.rs @@ -175,3 +175,21 @@ fn test_allowlist_escape() -> Result<()> { Ok(()) } + +#[cfg(target_os = "windows")] +#[test] +fn test_windows_allowlists_are_not_case_sensitive() -> Result<()> { + let allowlist = AllowList::parse("vccrt")?; + assert!(allowlist.is_allowed("VCCRT")); + + Ok(()) +} + +#[cfg(not(target_os = "windows"))] +#[test] +fn test_linux_allowlists_are_case_sensitive() { + let allowlist = AllowList::parse("vccrt")?; + assert!(!allowlist.is_allowed("VCCRT")); + + Ok(()) +} diff --git a/src/agent/coverage/src/source.rs b/src/agent/coverage/src/source.rs index b556fe447a..397dd4b744 100644 --- a/src/agent/coverage/src/source.rs +++ b/src/agent/coverage/src/source.rs @@ -104,13 +104,18 @@ pub fn binary_to_source_coverage( }; if let Some(file) = location.file() { + let file_path = if cfg!(windows) { + // Windows paths are case insensitive. + FilePath::new(file.full_path().to_ascii_lowercase())? + } else { + FilePath::new(file.full_path())? + }; + // Only include relevant inlinees. - if !source_allowlist.is_allowed(&file.full_path()) { + if !source_allowlist.is_allowed(&file_path) { continue; } - let file_path = FilePath::new(file.full_path())?; - // We have a hit. let file_coverage = source.files.entry(file_path).or_default(); let line = Line(line_number); diff --git a/src/agent/coverage/tests/snapshot.rs b/src/agent/coverage/tests/snapshot.rs index f929663925..0a6d5cb302 100644 --- a/src/agent/coverage/tests/snapshot.rs +++ b/src/agent/coverage/tests/snapshot.rs @@ -53,21 +53,21 @@ fn windows_snapshot_tests() { .record() .unwrap(); - // Windows modules should be case insensitive - recorded - .coverage - .modules - .keys() - .for_each(|k| assert_eq!(k.to_string().to_ascii_lowercase(), k.to_string())); - // generate source-line coverage info let source = coverage::source::binary_to_source_coverage(&recorded.coverage, &source_allowlist) .expect("binary_to_source_coverage"); + // For Windows, the source coverage is tracked using case-insensitive paths. + // The conversion from case-sensitive to insensitive is done when converting from binary to source coverage. + // By naming our test file with a capital letter, we can ensure that the case-insensitive conversion is working. + source.files.keys().for_each(|k| { + assert_eq!(k.to_string().to_ascii_lowercase(), k.to_string()); + }); + let file_coverage = source .files - .get(&FilePath::new(input_path.to_string_lossy()).unwrap()) + .get(&FilePath::new(input_path.to_string_lossy().to_ascii_lowercase()).unwrap()) .expect("coverage for input"); let mut result = String::new(); diff --git a/src/agent/debugger/src/module.rs b/src/agent/debugger/src/module.rs index 8a55be4702..038728a774 100644 --- a/src/agent/debugger/src/module.rs +++ b/src/agent/debugger/src/module.rs @@ -46,9 +46,7 @@ impl Module { error!("Error getting path from file handle: {}", e); "???".into() }); - - let path = PathBuf::from(path.as_os_str().to_ascii_lowercase()); - + let image_details = get_image_details(&path)?; Ok(Module { From 76e95039f869b1ad913697df50bb8dd894b39a2f Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 15:40:23 -0400 Subject: [PATCH 4/9] fmt --- src/agent/coverage/src/allowlist.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/agent/coverage/src/allowlist.rs b/src/agent/coverage/src/allowlist.rs index 9371578480..2c67130375 100644 --- a/src/agent/coverage/src/allowlist.rs +++ b/src/agent/coverage/src/allowlist.rs @@ -145,8 +145,7 @@ fn glob_to_regex(expr: &str) -> Result { // On Windows we should also ignore case. let expr = if cfg!(windows) { format!("(?i)^{expr}$") - } - else { + } else { format!("^{expr}$") }; From d94efb5c2076b34b7c09c4376dee052779fdd99d Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 15:56:13 -0400 Subject: [PATCH 5/9] Debugging missing file coverage --- src/agent/coverage/tests/snapshot.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/agent/coverage/tests/snapshot.rs b/src/agent/coverage/tests/snapshot.rs index 0a6d5cb302..7c6cb301b4 100644 --- a/src/agent/coverage/tests/snapshot.rs +++ b/src/agent/coverage/tests/snapshot.rs @@ -58,6 +58,8 @@ fn windows_snapshot_tests() { coverage::source::binary_to_source_coverage(&recorded.coverage, &source_allowlist) .expect("binary_to_source_coverage"); + println!("{:?}", source.files.keys()); + // For Windows, the source coverage is tracked using case-insensitive paths. // The conversion from case-sensitive to insensitive is done when converting from binary to source coverage. // By naming our test file with a capital letter, we can ensure that the case-insensitive conversion is working. From 127b54eed887930b6dcfbd6e4334aad3bfc5964e Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 16:27:52 -0400 Subject: [PATCH 6/9] fmt --- src/agent/debugger/src/module.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/agent/debugger/src/module.rs b/src/agent/debugger/src/module.rs index 038728a774..aefdb8a92e 100644 --- a/src/agent/debugger/src/module.rs +++ b/src/agent/debugger/src/module.rs @@ -46,7 +46,6 @@ impl Module { error!("Error getting path from file handle: {}", e); "???".into() }); - let image_details = get_image_details(&path)?; Ok(Module { From 874bbb046323e0c51451fa96bad80e297b73e110 Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Mon, 25 Sep 2023 16:51:40 -0400 Subject: [PATCH 7/9] Broken linux test --- src/agent/coverage/src/allowlist/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/agent/coverage/src/allowlist/tests.rs b/src/agent/coverage/src/allowlist/tests.rs index 8de5448d5a..8d22d93962 100644 --- a/src/agent/coverage/src/allowlist/tests.rs +++ b/src/agent/coverage/src/allowlist/tests.rs @@ -187,7 +187,7 @@ fn test_windows_allowlists_are_not_case_sensitive() -> Result<()> { #[cfg(not(target_os = "windows"))] #[test] -fn test_linux_allowlists_are_case_sensitive() { +fn test_linux_allowlists_are_case_sensitive() -> Result<()> { let allowlist = AllowList::parse("vccrt")?; assert!(!allowlist.is_allowed("VCCRT")); From 68a3fe2d83ed4c0c1d59b1bf065707d73718f32e Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Tue, 26 Sep 2023 10:26:46 -0400 Subject: [PATCH 8/9] Add a case insensitive transformer for better perf --- src/agent/coverage/src/source.rs | 37 +++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/agent/coverage/src/source.rs b/src/agent/coverage/src/source.rs index 397dd4b744..78b8e79f79 100644 --- a/src/agent/coverage/src/source.rs +++ b/src/agent/coverage/src/source.rs @@ -2,6 +2,7 @@ // Licensed under the MIT License. use std::collections::{BTreeMap, BTreeSet}; +use std::convert; use std::num::NonZeroU32; use anyhow::{Context, Result}; @@ -11,6 +12,7 @@ use debuggable_module::load_module::LoadModule; use debuggable_module::loader::Loader; use debuggable_module::path::FilePath; use debuggable_module::{Module, Offset}; +use symbolic::symcache::transform::{SourceLocation, Transformer}; use crate::allowlist::AllowList; use crate::binary::BinaryCoverage; @@ -69,6 +71,30 @@ pub fn binary_to_source_coverage( let mut symcache = vec![]; let mut converter = SymCacheConverter::new(); + if cfg!(windows) { + use symbolic::symcache::transform::Function; + struct CaseInsensitive {} + impl Transformer for CaseInsensitive { + fn transform_function<'f>(&'f mut self, f: Function<'f>) -> Function<'f> { + f + } + + fn transform_source_location<'f>( + &'f mut self, + mut sl: SourceLocation<'f>, + ) -> SourceLocation<'f> { + sl.file.name = sl.file.name.to_ascii_lowercase().into(); + sl.file.directory = sl.file.directory.map(|d| d.to_ascii_lowercase().into()); + sl.file.comp_dir = sl.file.comp_dir.map(|d| d.to_ascii_lowercase().into()); + sl + } + } + + let case_insensitive_transformer = CaseInsensitive {}; + + converter.add_transformer(case_insensitive_transformer); + } + let exe = Object::parse(module.executable_data())?; converter.process_object(&exe)?; @@ -104,18 +130,13 @@ pub fn binary_to_source_coverage( }; if let Some(file) = location.file() { - let file_path = if cfg!(windows) { - // Windows paths are case insensitive. - FilePath::new(file.full_path().to_ascii_lowercase())? - } else { - FilePath::new(file.full_path())? - }; - // Only include relevant inlinees. - if !source_allowlist.is_allowed(&file_path) { + if !source_allowlist.is_allowed(&file.full_path()) { continue; } + let file_path = FilePath::new(file.full_path())?; + // We have a hit. let file_coverage = source.files.entry(file_path).or_default(); let line = Line(line_number); From b67e3bb54c75463e9eda05f6dcbea75559a516ff Mon Sep 17 00:00:00 2001 From: Teo Voinea Date: Tue, 26 Sep 2023 10:27:25 -0400 Subject: [PATCH 9/9] cargo fix --- src/agent/coverage/src/source.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/agent/coverage/src/source.rs b/src/agent/coverage/src/source.rs index 78b8e79f79..e06e8aa285 100644 --- a/src/agent/coverage/src/source.rs +++ b/src/agent/coverage/src/source.rs @@ -2,7 +2,7 @@ // Licensed under the MIT License. use std::collections::{BTreeMap, BTreeSet}; -use std::convert; + use std::num::NonZeroU32; use anyhow::{Context, Result};