Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Commit

Permalink
Make modules case insenstive on windows (#3527)
Browse files Browse the repository at this point in the history
* Make modules and coverage allowlist case insensitive on Windows

* Tests and fmt

* PR comments

* fmt

* Debugging missing file coverage

* fmt

* Broken linux test

* Add a case insensitive transformer for better perf

* cargo fix
  • Loading branch information
tevoinea authored and chkeita committed Oct 3, 2023
1 parent 7bcc41c commit 9132e11
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 8 deletions.
7 changes: 6 additions & 1 deletion src/agent/coverage/src/allowlist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,12 @@ fn glob_to_regex(expr: &str) -> Result<Regex> {
let expr = expr.replace(r"\*", ".*");

// Anchor to line start and end.
let expr = format!("^{expr}$");
// On Windows we should also ignore case.
let expr = if cfg!(windows) {
format!("(?i)^{expr}$")
} else {
format!("^{expr}$")
};

Ok(Regex::new(&expr)?)
}
Expand Down
18 changes: 18 additions & 0 deletions src/agent/coverage/src/allowlist/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() -> Result<()> {
let allowlist = AllowList::parse("vccrt")?;
assert!(!allowlist.is_allowed("VCCRT"));

Ok(())
}
26 changes: 26 additions & 0 deletions src/agent/coverage/src/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

use std::collections::{BTreeMap, BTreeSet};

use std::num::NonZeroU32;

use anyhow::{Context, Result};
Expand All @@ -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;
Expand Down Expand Up @@ -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)?;

Expand Down
14 changes: 12 additions & 2 deletions src/agent/coverage/tests/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -57,9 +58,18 @@ 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.
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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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 <iostream>
[ ]
Expand Down
1 change: 0 additions & 1 deletion src/agent/debugger/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ impl Module {
error!("Error getting path from file handle: {}", e);
"???".into()
});

let image_details = get_image_details(&path)?;

Ok(Module {
Expand Down
9 changes: 6 additions & 3 deletions src/agent/onefuzz/src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ impl<'a> Expand<'a> {

fn input_file_sha256(&self) -> Result<ExpandedValue<'a>> {
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(),
)
Expand All @@ -149,7 +150,8 @@ impl<'a> Expand<'a> {

fn extract_file_name_no_ext(&self) -> Result<ExpandedValue<'a>> {
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(),
)
Expand All @@ -173,7 +175,8 @@ impl<'a> Expand<'a> {

fn extract_file_name(&self) -> Result<ExpandedValue<'a>> {
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(),
)
Expand Down

0 comments on commit 9132e11

Please sign in to comment.